feat: infer timestamp constraint for single-timestamp column #1266
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
This commit addresses issue apache#1144. It aims to automatically infer the timestamp column during table creation when possible. Previously, users had to explicitly set a timestamp constraint, adding unnecessary complexity. Now, the system will detect if there is only one timestamp column and set it as the constraint. - Added `build_and_check_timestamp_key_constraint` to check and infer. - Modified `create_table` to call the new function.
- Refactor test cases for `create_table` in `parser.rs` to align with the latest timestamp inference changes made in the previous commit. - Update the expected statements to include unique constraints for timestamp columns, as influenced by the modifications in timestamp inference.
jiacai2050
reviewed
Oct 17, 2023
jiacai2050
reviewed
Oct 17, 2023
|
The general implementation looks GOOD to me. 👍 I leave a few minor suggestions, do those make sense to you? |
- Rename from `build_and_check_timestamp_key_constraint` to `build_or_infer_timestamp_key_constraint` - Modify the function's comment
- Added a test case that includes two timestamp columns and explicitly selects the c2 field as the timestamp key. The test passes locally.
|
Thank you for the review and for your positive feedback! 👍 I've gone ahead and made some additional updates based on our discussion:
Your minor suggestions make complete sense to me, and I've taken them into account. Here's the link to the updated CI results. Please let me know if there's anything else to improve. |
jiacai2050
changed the title
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.
Rationale
This PR aims to resolve issue #1144 by simplifying the process of table creation with timestamp constraints. Specifically, the system will automatically infer and set the timestamp constraint under the following conditions:
DataType::Timestamp).By introducing this automatic inference, we reduce the complexity for users who otherwise would have to manually set this constraint.
Detailed Changes
Automatic Inference of Timestamp Constraint: The system now detects if there is only a single timestamp column during table creation and automatically sets it as the timestamp constraint.
build_and_check_timestamp_key_constraintfor checking and inference.create_tablefunction to call the new function.Updated Test Expectations for
create_table:create_tableinparser.rsto align with the latest timestamp inference changes.Test Plan
Utilized the default CI Action for testing. The result was a pass.
Remarks