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

Initial Snowflake migration docs #1538

Merged
merged 4 commits into from
Oct 30, 2023
Merged

Initial Snowflake migration docs #1538

merged 4 commits into from
Oct 30, 2023

Conversation

justindeguzman
Copy link
Contributor

@justindeguzman justindeguzman commented Sep 22, 2023

@robot-clickhouse
Copy link
Member

robot-clickhouse commented Sep 22, 2023

This is an automated comment for commit f659664 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Successful checks
Check nameDescriptionStatus
VercelThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Check nameDescriptionStatus
Docs CheckBuilds and tests the documentation❌ failure

@justindeguzman justindeguzman marked this pull request as ready for review September 22, 2023 16:26
Copy link
Collaborator

@kellytoole kellytoole left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you so much again

@tbragin
Copy link
Member

tbragin commented Oct 17, 2023

@gingerwizard any reviews? what are the next steps here? cc @Ryado

SELECT
timestamp,
some_text,
JSONExtract(
Copy link
Contributor

Choose a reason for hiding this comment

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

last time i checked they do export OBJECT as JSON string..this is subject to change. i raised a question and they planned to possibly change. Maybe add note?

```

:::note Note on nested column structures
The `VARIANT` and `OBJECT` columns in the original Snowflake table schema will be output as JSON strings by default, forcing us to cast these when inserting them into ClickHouse.
Copy link
Contributor

Choose a reason for hiding this comment

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

note subject to change

Copy link
Contributor

@gingerwizard gingerwizard left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe note the string export of JSON is subject to change and users should do a DESCRIBE s3() toi check the schema

@justindeguzman
Copy link
Contributor Author

LGTM. Maybe note the string export of JSON is subject to change and users should do a DESCRIBE s3() toi check the schema

Just to keep it simple, I don't think we should add this info unless Snowflake's behavior actually changes.

Thanks for reviewing @gingerwizard!

@justindeguzman justindeguzman merged commit d4f3422 into main Oct 30, 2023
2 of 3 checks passed
@justindeguzman justindeguzman deleted the snowflake-migration branch November 7, 2023 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants