Source Firebolt: allow reading from views #16583
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/connectors
|
Hi @ptiurin thanks for the PR. This PR is part of Airbyte's Community Maintainer program and someone from our community will be assigned to review your PR and help it get merged. Thanks for being patient and thanks for improving this connector. |
|
/test connector=connectors/source-firebolt |
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
|
@ptiurin |
|
@YiyangLi , Looks like the database you have is slightly out of date. I'll get back to you once the version is updated. That should fix the tests as well. |
|
@YiyangLi , upgrade is done. If you have access to Airbyte's Firebolt account please stop and start the engine again. This will update it and the tests would pass once it's up. |
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
|
|
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
@ptiurin sorry for the delayed response. Looks like the tests fail, I could try explain it. First failureIt looks like there is a txt file called expected_records.txt, the records read from the database doesn't match the one in the static file. Can you help update the file? Second failureIt looks like the read fails because of mismatch format on column5, the one under Let me know if you have a question. Once the change is made, I will run the tests again. Thanks. |
|
@ptiurin can you take a look at the comments above? |
|
Apologies for the delay @sajarin @YiyangLi , The catalog and expected records files are correct. and For the first one I'll push a change to get schemas more efficiently. For the second, the fix is on the database side, as it's a bug. I'll let you know once it's fixed in the airbyte FIrebolt account. |
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
|
@YiyangLi your Firebolt account is updated now. I've also rebased the branch on master. The tests are succeeding locally, could you re-run the acceptance tests? |
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
YiyangLi
approved these changes
Oct 31, 2022
|
@ptiurin I can't help you rebase the branch. Can you either grant me access or rebase it yourself? |
|
The test in CI fails because of a dependency conflict. I can't push a commit for you, see if the following suggestion works: Thanks. |
|
Thanks @YiyangLi , I've added your suggestion. I've also granted you write access so you can experiment if it doesn't work. |
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
|
@ptiurin Can you advise how to address the rate limit issue when CI runs the test? 2 tests fail, and the reason is the same -- 429 too many requests. I didn't run the tests too frequently, I didn't run it yesterday, and I run it once today, I assume the quota is recharged. The CDK offers a retry if |
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
|
@YiyangLi Looking at the logs, it's running the old version of code, which did individual calls per table. (If you search for SHOW COLUMNS in the logs, that's the code that's been deleted in this PR). Is there some sort of caching? Maybe a way to rebuild a docker image to pick up the latest code from this PR? |
|
/test connector=connectors/source-firebolt
Build FailedTest summary info: |
|
@ptiurin I rebuilt the docker image in my local environment, with the flag of Build the imageErrorsDetailsThis is the same error in CI, I think CI builds an image from the scratch everytime it runs the test. |
|
@YiyangLi I think I finally understand where the error is coming from. Backwards compatibility test runs both new and old image to compare them. The old image contains the old code, which queries the structure of each table one-by-one. Airbyte database contains a large number of tables so this overwhelms the API with many simultaneous requests. The new image contains updated code that fetches structures of any number of tables in one go, so this issue does not occur. |
@sajarin Can you ask around to see if these tables are used? If not, can I drop these tables? The following screenshot says, 661 out of 708 tables contain "_airbyte_raw".
According to the configured_catalog.json, only 1 table is read for the integration test. But before that, the connector will run the following query to get all available tables and columns, which throws 429 too many requests error in the CI test. I want confirm with the airbyte team. If it's safe to drop these tables, we can get the CI test pass. SELECT table_name, column_name, data_type, is_nullable FROM information_schema.columns
WHERE table_name NOT IN (
SELECT table_name FROM information_schema.tables WHERE table_type IN ('EXTERNAL', 'CATALOG')
) |
|
/test connector=connectors/source-firebolt
Build PassedTest summary info: |
|
/test connector=connectors/source-firebolt
Build PassedTest summary info: |
|
/publish connector=connectors/source-firebolt
if you have connectors that successfully published but failed definition generation, follow step 4 here |
|
Hello 👋, first thank you for this amazing contribution. We really appreciate the effort you've made to improve the project. If you have any questions feel free to send me a message in Slack! |
marcosmarxm
requested changes
Dec 1, 2022
There was a problem hiding this comment.
@YiyangLi it is missing the seed file generation. Check this step here: https://docs.airbyte.com/connector-development/#publishing-a-connector
.pre-commit-config.yaml
Outdated
| @@ -25,7 +25,7 @@ repos: | |||
| ] | |||
| additional_dependencies: ["colorama"] | |||
| - repo: https://github.com/pre-commit/mirrors-prettier | |||
| rev: v2.5.0 | |||
| rev: v3.0.0-alpha.1 | |||
There was a problem hiding this comment.
@YiyangLi this files don't belong to this change. Please revert this.
marcosmarxm
requested changes
Dec 1, 2022
| | 0.2.0 | 2022-09-09 | TBD | Reading from views | |
There was a problem hiding this comment.
yep, updated the doc. I am going to run test again. Can you help remove the docker image before I run the publish?
- revert .pre-commit-config.yaml - update versions manually under the seed file - update the doc to reflect the PR link
|
/test connector=connectors/source-firebolt
Build PassedTest summary info: |
|
/publish connector=connectors/source-firebolt
if you have connectors that successfully published but failed definition generation, follow step 4 here |
|
/publish connector=connectors/source-firebolt
if you have connectors that successfully published but failed definition generation, follow step 4 here |
marcosmarxm
approved these changes
Dec 2, 2022
marcosmarxm
requested changes
Dec 7, 2022
| @@ -55,7 +55,8 @@ You can now use the Airbyte Firebolt source. | |||
|
|
|||
| ## Changelog | |||
|
|
|||
|
|
|||
| .pre-commit-config.yaml | |||
marcosmarxm
approved these changes
Dec 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Allow using views the same way as tables to read data from in the Source. Views now appear in the same list in Replication settings in UI.
How
Changing the query that only shows tables and their columns to the one that shows columns for views as well.
Recommended reading order
database.pysource.py🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Fixed a bug where ARRAY(INT) whould be
{"type": "array", "items": {"type": ["null", "string"]}}instead of{"type": "array", "items": {"type": ["null", "integer"]}}Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereTests
Unit
Integration
Acceptance
Backwards-compatibility's failing due to the bugfix described above.