Remove obsolete and deprecated bigquery native read.

[robertwb]

Defers to builtin read with a warning rather than raising an exception.

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Choose reviewer(s) and mention them in a comment (R: @username).
• Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
• Update CHANGES.md with noteworthy changes.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

[robertwb]

R: @Abacn
CC: @pabloem

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

[Abacn]

Thanks! Is there an GitHub Issue for the deprecation? If not could we create one and refer to it in CHANGES?

[robertwb]

Thanks! Is there an GitHub Issue for the deprecation? If not could we create one and refer to it in CHANGES?

Done.

[codecov]

Codecov Report

Merging #23557 (03953e8) into master (0e61b02) will decrease coverage by 0.11%.
The diff coverage is 33.33%.

❗ Current head 03953e8 differs from pull request most recent head bbf3b81. Consider uploading reports for the commit bbf3b81 to get more accurate results

@@            Coverage Diff             @@
##           master   #23557      +/-   ##
==========================================
- Coverage   73.46%   73.34%   -0.12%     
==========================================
  Files         718      718              
  Lines       95935    95883      -52     
==========================================
- Hits        70477    70327     -150     
- Misses      24147    24245      +98     
  Partials     1311     1311              

Flag	Coverage Δ
python	83.05% <33.33%> (-0.16%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...on/apache_beam/runners/dataflow/dataflow_runner.py	83.40% <ø> (+0.48%)	⬆️
sdks/python/apache_beam/io/gcp/bigquery.py	73.13% <33.33%> (-1.11%)	⬇️
sdks/python/apache_beam/io/gcp/bigquery_tools.py	72.12% <0.00%> (-13.62%)	⬇️
sdks/python/apache_beam/internal/gcp/json_value.py	85.50% <0.00%> (-2.90%)	⬇️
sdks/python/apache_beam/runners/runner.py	72.39% <0.00%> (-1.85%)	⬇️
sdks/python/apache_beam/runners/direct/executor.py	96.46% <0.00%> (-0.55%)	⬇️
sdks/python/apache_beam/io/iobase.py	85.92% <0.00%> (-0.50%)	⬇️
...ks/python/apache_beam/runners/worker/sdk_worker.py	89.08% <0.00%> (-0.17%)	⬇️
... and 6 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Abacn]

One thing I notice is that the removed bigquery_tools.BigQueryReader class is a documented one (documentation) and never marked as deprecated, although classes using it were marked as deprecated (bigquery._BigQuerySource and bigquery.BigQueryReader). bigquery_tools.py also had no doctoring warning the module was for internal usage only.

I would recommend we mark bigquery_tools.BigQueryReader class as deprecated for now, and remove it and its tests in a future version (after upcoming 2.43.0).

The changes in bigquery.py and its tests look good to me.

EDIT: looks like tests for bigquery_tools.BigQueryReader depends on bigquery.BigQueryReader so they live together. Looking for @pabloem who may have more contexts on this if it is safe to remove this class.

[robertwb]

OK, I've left BigQueryReader for now, though technically it's really an internal-only class. We'll remove it later. Its only public entrypoint is already marked as deprecated since 2.11.0. (It didn't have any self-contained unit tests, and it didn't seem worth refactoring the other tests at this point.)

[Abacn]

Thanks for clarifying that this is an internal-only class. In this case I agree it is good time to remove it now. And I think better remove both than removing tests and leave the class alone. The commit bbf3b81 looks good to me.

[robertwb]

The failure at apache_beam.runners.portability.flink_runner_test.FlinkRunnerTestOptimized.test_assert_that looks irrelevent.

[robertwb]

Run Python_PVR_Flink PreCommit

[Abacn]

Hi robertwb@, I meant I understand now the original approach (also remove bigquery_tools.BigQueryReader) makes sense to me than my first comment. Sorry for confusion

[Abacn]

LGTM, thanks!