feat: provide TableDefinition functions for Iceberg tables
#5881
Conversation
This file contains hidden or 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
lbooker42
commented
Aug 1, 2024
extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergCatalogAdapter.java
Show resolved
Hide resolved
rcaudy
reviewed
Aug 1, 2024
| // Map all the column names in the schema to their legalized names. | ||
| final Map<String, String> legalizedColumnRenames = new HashMap<>(); | ||
|
|
||
| // Validate user-supplied names meet legalization instructions |
Member
There was a problem hiding this comment.
Suggested change
| // Validate user-supplied names meet legalization instructions | |
| // Validate user-supplied names meet legalization rules |
| * @param instructions The instructions for customizations while reading | ||
| * @return The table definition as a Deephaven table | ||
| */ | ||
| @SuppressWarnings("unused") |
Member
There was a problem hiding this comment.
Maybe we want @ScriptAPI annotations for this stuff?
extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergCatalogAdapter.java
Show resolved
Hide resolved
devinrsmith
reviewed
Aug 1, 2024
| final String destinationName = entry.getValue(); | ||
| if (!NameValidator.isValidColumnName(destinationName)) { | ||
| throw new TableDataException( | ||
| String.format("%s - invalid column name provided (%s)", table, destinationName)); |
Member
There was a problem hiding this comment.
We are losing the snapshotId that we had previously; we could still pass snapshotId, and have it be -1 when there is no snapshot, or something.
devinrsmith
reviewed
Aug 1, 2024
Comment on lines
+721
to
+722
| uploadParquetFiles(new File(IcebergToolsTest.class.getResource("/warehouse/sales/sales_multi").getPath()), | ||
| warehousePath); |
Member
There was a problem hiding this comment.
Do we need to upload parquet files to get the definition? I think it's all involved in the metadata (which we aren't uploading as part of this).
Comment on lines
+734
to
+738
| tableDef.checkHasColumn("Region", String.class); | ||
| tableDef.checkHasColumn("Item_Type", String.class); | ||
| tableDef.checkHasColumn("Units_Sold", int.class); | ||
| tableDef.checkHasColumn("Unit_Price", double.class); | ||
| tableDef.checkHasColumn("Order_Date", Instant.class); |
Member
There was a problem hiding this comment.
I think we should prefer to model this as
TableDefinition expected = ...;
TableDefinition actual = adapter.getTableDefinition(...);
assertEquals(expected, actual);or something like that
Member
|
Completed via #5891 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Will close #5868 when merged (but needs reconciled with #5880)