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 Snowflake source #20138

Merged
merged 6 commits into from
Dec 8, 2022

Conversation

DoNotPanicUA
Copy link
Contributor

@DoNotPanicUA DoNotPanicUA commented Dec 6, 2022

What

Enable SAT for the Snowflake source

Configured test instances

  • sat_test_dataset

Found issues

SAT related issues:

  • The source returns time data with milliseconds, but the SAT treats this as a type violation. - ⚠️ Shall we trim the data in the source?
  • The SAT fails when face non-typical values like Nan, +inf or -inf

@DoNotPanicUA DoNotPanicUA linked an issue Dec 6, 2022 that may be closed by this pull request
@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Dec 6, 2022

/test connector=connectors/source-snowflake

🕑 connectors/source-snowflake https://github.com/airbytehq/airbyte/actions/runs/3630718406
✅ connectors/source-snowflake https://github.com/airbytehq/airbyte/actions/runs/3630718406
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 73.20s (0:01:13) =============

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

DoNotPanicUA commented Dec 7, 2022

/test connector=connectors/source-snowflake

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

@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Dec 7, 2022

/test connector=connectors/source-snowflake

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

@DoNotPanicUA
Copy link
Contributor Author

@alafanechere
The CI run fails with the new format, but the local pass is fine.
Screenshot from 2022-12-07 17-05-27
any ideas on how to fix it?

@alafanechere
Copy link
Contributor

alafanechere commented Dec 7, 2022

I think this is because you needed a branch update after #20076

@alafanechere
Copy link
Contributor

alafanechere commented Dec 7, 2022

/test connector=connectors/source-snowflake

🕑 connectors/source-snowflake https://github.com/airbytehq/airbyte/actions/runs/3640202284
❌ connectors/source-snowflake https://github.com/airbytehq/airbyte/actions/runs/3640202284
🐛 https://gradle.com/s/zxvbkyeixktx6

Build Failed

Test summary info:

Could not find result summary

@DoNotPanicUA
Copy link
Contributor Author

@alafanechere
Looks like we can't set the strictness level as High because it requires the future cursor check.
This check doesn't work for the java sources because they use the old State massage handler, and it requires more deep investigation with the significant refactoring of the JDBC sources.

@alafanechere
Copy link
Contributor

Looks like we can't set the strictness level as High because it requires the future cursor check.
This check doesn't work for the java sources because they use the old State massage handler, and it requires more deep investigation with the significant refactoring of the JDBC sources.

It makes sense, thank you for trying!

@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Dec 7, 2022

/test connector=connectors/source-snowflake

🕑 connectors/source-snowflake https://github.com/airbytehq/airbyte/actions/runs/3640361840
✅ connectors/source-snowflake https://github.com/airbytehq/airbyte/actions/runs/3640361840
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 70.21s (0:01:10) ===================

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)

@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Dec 8, 2022

/test connector=connectors/source-snowflake

🕑 connectors/source-snowflake https://github.com/airbytehq/airbyte/actions/runs/3647774642
✅ connectors/source-snowflake https://github.com/airbytehq/airbyte/actions/runs/3647774642
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 69.06s (0:01:09) ===================

@DoNotPanicUA DoNotPanicUA merged commit 9eb0f0d into master Dec 8, 2022
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 - Snowflake
3 participants