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

Rewords comment describing WRITETIME/TTL settings #238

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arvy
Copy link

@arvy arvy commented Dec 18, 2023

What this PR does:

Comment rewording based on feedback from a previous PR (the other PR was merged before I had a chance to make this change)

@arvy arvy requested a review from a team as a code owner December 18, 2023 22:51
#spark.cdm.schema.origin.column.ttl.automatic true

# Max TTL value of specified non-PK columns will be used for insert on target (overrides automatic setting)
Copy link
Collaborator

@msmygit msmygit Dec 19, 2023

Choose a reason for hiding this comment

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

Curious, why are we removing non-PK verbiage from here & line 114 as there won't be any writetime/ttl for primary key columns of a C* table?

Copy link
Author

Choose a reason for hiding this comment

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

The specified columns are implicitly non-PK columns - if they are not, validation will fail and feature will be disabled :)

Copy link
Author

Choose a reason for hiding this comment

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

I can add it back in (in parentheses)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should add it back as only non-PK columns has the writimetime/ttl values/properties.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arvy @arvydasj could you pls take care of this trivial doc update pr? Thank you in advance!

@msmygit msmygit enabled auto-merge (squash) February 29, 2024 19:34
auto-merge was automatically disabled March 19, 2024 19:32

Head branch was pushed to by a user without write access

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.

3 participants