Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable full SAT for the BigQuery source #19720

Merged
merged 13 commits into from
Dec 8, 2022

Conversation

DoNotPanicUA
Copy link
Contributor

@DoNotPanicUA DoNotPanicUA commented Nov 22, 2022

What

Enable SAT for the BigQuery source

Added secrets:

  • SECRET_SOURCE-BIGQUERY_SAT__CREDS

Configured test instances

  • [BigQuery] sat_test_dataset

Found issues

SAT related issues:

  • SAT can't check null values due to column type validation fail

Source related issues:

  • [BigQuery] Complex type like REPEATED or OBJECT fail validation. The root cause of the issue requires investigation. Is it SAT issue, or is the source work wrong?
  • [BigQuery] Fails the future incremental check. An investigation is required. Or the abnormal cursor value format is wrong, or the source doesn't accept the initial state message.

@DoNotPanicUA

This comment was marked as outdated.

@DoNotPanicUA

This comment was marked as outdated.

@DoNotPanicUA

This comment was marked as outdated.

@DoNotPanicUA DoNotPanicUA self-assigned this Nov 28, 2022
@DoNotPanicUA

This comment was marked as outdated.

@DoNotPanicUA DoNotPanicUA changed the title enable full SAT for the BigQuery source [SAT] Enable full SAT for the global sources Nov 29, 2022
@DoNotPanicUA

This comment was marked as off-topic.

@DoNotPanicUA DoNotPanicUA changed the title [SAT] Enable full SAT for the global sources [SAT] Enable full SAT for the BigQuery source Nov 30, 2022
@DoNotPanicUA DoNotPanicUA changed the title [SAT] Enable full SAT for the BigQuery source Enable full SAT for the BigQuery source Nov 30, 2022
@DoNotPanicUA DoNotPanicUA force-pushed the aleonets/19708-enable-full-sat-host-src branch from 95ded5f to e8d7254 Compare November 30, 2022 11:58
@DoNotPanicUA

This comment was marked as outdated.

@DoNotPanicUA

This comment was marked as outdated.

@DoNotPanicUA DoNotPanicUA marked this pull request as ready for review November 30, 2022 15:53
@DoNotPanicUA DoNotPanicUA requested a review from a team as a code owner November 30, 2022 15:53
@DoNotPanicUA DoNotPanicUA requested review from evantahler and a team November 30, 2022 15:53
@DoNotPanicUA
Copy link
Contributor Author

Yes!

I'm curious how the BQ instance for testing was setup. Other than in the Google Secrets, should we document the table/database/user anywhere?

It would be very useful to store all links to the credentials and creation of test instances.
So far, I store all scripts in the repository like here.
We could create a doc as they don't contain any sensitive information.

@evantahler
Copy link
Contributor

evantahler commented Dec 1, 2022

/test connector=connectors/source-bigquery

🕑 connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3596550597
❌ connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3596550597
🐛

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:26: `future_state` not specified, skipping.
============ 29 passed, 1 skipped, 31 warnings in 61.86s (0:01:01) =============

Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you commit the SQL files I would suggest documenting how you provisioned BigQuery with the test data. It's strange that the /test reported status is "failing" while the tests have passed. I'm going to re-run it to check if it's a transient problem.

@alafanechere
Copy link
Contributor

alafanechere commented Dec 2, 2022

/test connector=connectors/source-bigquery

🕑 connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3601749168
❌ connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3601749168
🐛

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:26: `future_state` not specified, skipping.
================= 29 passed, 1 skipped, 31 warnings in 57.88s ==================

@DoNotPanicUA
Copy link
Contributor Author

image

@alafanechere
Copy link
Contributor

@DoNotPanicUA would you mind trying to update your branch? I'm wondering if the Report status failure is due to recent changes on the AMI (cc @evantahler )

@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Dec 2, 2022

/test connector=connectors/source-bigquery

🕑 connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3603180470
✅ connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3603180470
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       140      5    96%   87, 93, 238, 242-243
	 source_acceptance_test/conftest.py                     199     93    53%   35, 41-43, 48, 53, 59, 65, 71, 77-79, 98, 103-105, 111-113, 119-120, 125-126, 131, 137, 146-155, 161-166, 181, 205, 236, 242, 250-255, 263-268, 276-289, 294-300, 307-318, 325-341
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              398    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 299, 337-354, 363-371, 375-380, 386, 419-424, 462-469, 512-514, 517, 582-590, 602-605, 610, 666-667, 673, 676, 712-722, 735-760
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       112     50    55%   23-26, 32, 36, 39-68, 71-73, 76-78, 81-83, 86-88, 91-93, 96-114, 148-150
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1569    350    78%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:26: `future_state` not specified, skipping.
============ 29 passed, 1 skipped, 31 warnings in 60.49s (0:01:00) =============

@evantahler
Copy link
Contributor

evantahler commented Dec 2, 2022

@DoNotPanicUA would you mind trying to update your branch? I'm wondering if the Report status failure is due to recent changes on the AMI (cc @evantahler )

Yep! The AMI was fixed yesterday here #19949 for /test and /publish commands. Looking good now!

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Please create a script or a README under integration_tests to explain how to run the sql queries you committed.

@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Dec 7, 2022

/test connector=connectors/source-bigquery

🕑 connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3639933799
❌ connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3639933799
🐛

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for the reasons explained here: #19915 (comment)

@alafanechere
Copy link
Contributor

alafanechere commented Dec 7, 2022

/test connector=connectors/source-bigquery

🕑 connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3642803432
✅ connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3642803432
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       140      5    96%   87, 93, 238, 242-243
	 source_acceptance_test/conftest.py                     208     92    56%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-280, 288-301, 306-312, 319-330, 337-353
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              398    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 299, 337-354, 363-371, 375-380, 386, 419-424, 462-469, 512-514, 517, 582-590, 602-605, 610, 666-667, 673, 676, 712-722, 735-760
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1599    332    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:26: `future_state` not specified, skipping.
======================== 29 passed, 1 skipped in 59.41s ========================

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated your branch to get the test working again. Feel free to merge once they pass.

@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Dec 8, 2022

/test connector=connectors/source-bigquery

🕑 connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3647778836
✅ connectors/source-bigquery https://github.com/airbytehq/airbyte/actions/runs/3647778836
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       140      5    96%   87, 93, 238, 242-243
	 source_acceptance_test/conftest.py                     208     92    56%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-280, 288-301, 306-312, 319-330, 337-353
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              398    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 299, 337-354, 363-371, 375-380, 386, 419-424, 462-469, 512-514, 517, 582-590, 602-605, 610, 666-667, 673, 676, 712-722, 735-760
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1599    332    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:26: `future_state` not specified, skipping.
======================== 29 passed, 1 skipped in 56.75s ========================

@DoNotPanicUA DoNotPanicUA merged commit 36d4c92 into master Dec 8, 2022
@DoNotPanicUA DoNotPanicUA deleted the aleonets/19708-enable-full-sat-host-src branch December 8, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable SAT for hosted sources - BigQuery
4 participants