feat: View Metadata Builder #908
Open
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
c-thiel
commented
Jan 23, 2025
Comment on lines
+253
to
+261
| // ToDo Discuss: Similar to TableMetadata sort-order, Java does not add changes | ||
| // in this case. I prefer to add changes as the state of the builder is | ||
| // potentially mutated (`last_added_version_id`), thus we should record the change. | ||
| if self.last_added_version_id != Some(version_id) { | ||
| self.changes.push(ViewUpdate::AddViewVersion { | ||
| view_version: view_version.with_version_id(version_id), | ||
| }); | ||
| self.last_added_version_id = Some(version_id); | ||
| } |
There was a problem hiding this comment.
Java returns here:
So when you do a rollback to an earlier version, the change isn't recorded in Java. Java will only update the last_added_version_id if the version is added in the builder. I would assume that the change will be recorded in Java as well. Because I think the addInBuilder will only be true if you add it twice. I think I'm okay with this, but let's check with @nastra to be sure 👍
Comment on lines
+293
to
+309
| // ToDo Discuss: This check is not present in Java. | ||
| // The `TableMetadataBuilder` uses these checks in multiple places - also in Java. | ||
| // If we think delayed requests are a problem, I think we should also add it here. | ||
| if let Some(last) = self.metadata.version_log.last() { | ||
| // commits can happen concurrently from different machines. | ||
| // A tolerance helps us avoid failure for small clock skew | ||
| if view_version.timestamp_ms() - last.timestamp_ms() < -ONE_MINUTE_MS { | ||
| return Err(Error::new( | ||
| ErrorKind::DataInvalid, | ||
| format!( | ||
| "Invalid snapshot timestamp {}: before last snapshot timestamp {}", | ||
| view_version.timestamp_ms(), | ||
| last.timestamp_ms() | ||
| ), | ||
| )); | ||
| } | ||
| } |
Comment on lines
+344
to
+348
| // ToDo Discuss: In Java this uses `new_view_version.version_id()` instead. | ||
| // I believe 0 is more appropriate here, as the first version should always be 0. | ||
| // The TableMetadataBuilder also uses 0 for partition specs (and other `reuse_or_create_*` functions). | ||
| // Consistent behaviour between the two builders is desirable. | ||
| .unwrap_or(INITIAL_VIEW_VERSION_ID) |
Comment on lines
+367
to
+376
| // ToDo Discuss: Java does not add changes in this case. I prefer to add changes | ||
| // as the state of the builder is potentially mutated (`last_added_schema_id`), | ||
| // thus we should record the change. | ||
| if self.last_added_schema_id != Some(schema_id) { | ||
| self.changes.push(ViewUpdate::AddSchema { | ||
| schema: schema.clone().with_schema_id(schema_id), | ||
| last_column_id: None, | ||
| }); | ||
| self.last_added_schema_id = Some(schema_id); | ||
| } |
| let builder = builder_without_changes(); | ||
| let v1 = new_view_version(0, 1, "select * from ns.tbl"); | ||
|
|
||
| // ToDo Java: I think we should remove line 688 - 693 in Java TestVeiwMetadata.java as it does nothing. |
Fokko
previously approved these changes
Feb 3, 2025
| .get(VIEW_PROPERTY_REPLACE_DROP_DIALECT_ALLOWED) | ||
| .map_or( | ||
| VIEW_PROPERTY_REPLACE_DROP_DIALECT_ALLOWED_DEFAULT, | ||
| |value| value.to_lowercase() == "true", |
There was a problem hiding this comment.
Would it make sense to move this into some utility method?
Comment on lines
+253
to
+261
| // ToDo Discuss: Similar to TableMetadata sort-order, Java does not add changes | ||
| // in this case. I prefer to add changes as the state of the builder is | ||
| // potentially mutated (`last_added_version_id`), thus we should record the change. | ||
| if self.last_added_version_id != Some(version_id) { | ||
| self.changes.push(ViewUpdate::AddViewVersion { | ||
| view_version: view_version.with_version_id(version_id), | ||
| }); | ||
| self.last_added_version_id = Some(version_id); | ||
| } |
There was a problem hiding this comment.
Java returns here:
So when you do a rollback to an earlier version, the change isn't recorded in Java. Java will only update the last_added_version_id if the version is added in the builder. I would assume that the change will be recorded in Java as well. Because I think the addInBuilder will only be true if you add it twice. I think I'm okay with this, but let's check with @nastra to be sure 👍
Co-authored-by: Fokko Driesprong <[email protected]>
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.
This PR is not completely ready yet as I believe the current mechanism of view expiration is flawed.
I opened a PR in Java to demonstrate the problem and use for discussions:
apache/iceberg#12051
Feedback from anyone is welcome. I am not sure what the best solutions looks like.