Skip to content

fix: [iceberg] Add LogicalTypeAnnotation in ParquetColumnSpec #2000

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 15, 2025
Merged

fix: [iceberg] Add LogicalTypeAnnotation in ParquetColumnSpec #2000

merged 3 commits into from
Jul 15, 2025
+319 −6

Conversation

huaxingao
Copy link
Contributor

@huaxingao huaxingao commented Jul 8, 2025 •
edited
Loading

Which issue does this PR close?

Closes #.

Rationale for this change

In order to pass and reassemble the ColumnDescriptor from Iceberg to Comet, we need to include LogicalTypeAnnotation too.

What changes are included in this PR?

How are these changes tested?

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 51.31579% with 37 lines in your changes missing coverage. Please review.

Project coverage is 58.67%. Comparing base (f09f8af) to head (5fdd82c).
Report is 326 commits behind head on main.

Files with missing lines Patch % Lines
.../src/main/java/org/apache/comet/parquet/Utils.java 45.58% 31 Missing and 6 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #2000      +/-   ##
============================================
+ Coverage     56.12%   58.67%   +2.55%     
- Complexity      976     1253     +277     
============================================
  Files           119      134      +15     
  Lines         11743    13102    +1359     
  Branches       2251     2396     +145     
============================================
+ Hits           6591     7688    +1097     
- Misses         4012     4179     +167     
- Partials       1140     1235      +95

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

parthchandra
parthchandra reviewed Jul 8, 2025
View reviewed changes
Copy link
Contributor

@parthchandra parthchandra left a comment

Choose a reason for hiding this comment

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

Probably need to include ID as well in the type definition

@kazuyukitanimura
Copy link
Contributor

cc @hsiang-c
Does it make sense to include [iceberg] in the title to run the Iceberg CI?

@hsiang-c
Copy link
Contributor

hsiang-c commented Jul 9, 2025

@kazuyukitanimura yes.

We can consider merging #1987 to speed up CI, but need to fix some tests due to configuration.

@huaxingao huaxingao changed the title fix: Add LogicalTypeAnnotation in ParquetColumnSpec fix: [Iceberg] Add LogicalTypeAnnotation in ParquetColumnSpec Jul 9, 2025
@huaxingao huaxingao changed the title fix: [Iceberg] Add LogicalTypeAnnotation in ParquetColumnSpec fix: [iceberg] Add LogicalTypeAnnotation in ParquetColumnSpec Jul 9, 2025
@huaxingao huaxingao closed this Jul 9, 2025
@huaxingao huaxingao reopened this Jul 9, 2025
andygrove
andygrove reviewed Jul 10, 2025
View reviewed changes
common/src/main/java/org/apache/comet/parquet/Utils.java Outdated
Comment on lines 360 to 361
boolean isUTC = Boolean.parseBoolean(params.getOrDefault("isAdjustedToUTC", "true"));
String timeUnitStr = params.getOrDefault("unit", "MICROS");
Copy link
Member

Choose a reason for hiding this comment

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

The use of default values seems like it could be dangerous if the caller forgets to pass a parameter or has a typo in the key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks

andygrove
andygrove reviewed Jul 10, 2025
View reviewed changes
common/src/main/java/org/apache/comet/parquet/Utils.java

// DECIMAL
case "DecimalLogicalTypeAnnotation":
int scale = Integer.parseInt(params.get("scale"));
Copy link
Member

Choose a reason for hiding this comment

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

Should we add error handling to return a helpful error if these keys do not exist, rather than fail with NPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, makes sense to me. I have added a check.

@andygrove
Copy link
Member

@huaxingao Could you add some unit tests?

@kazuyukitanimura kazuyukitanimura merged commit b4ac876 into apache:main Jul 15, 2025
92 checks passed
@kazuyukitanimura
Copy link
Contributor

Thanks @huaxingao @andygrove @parthchandra @hsiang-c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andygrove andygrove andygrove left review comments

@parthchandra parthchandra parthchandra left review comments

@kazuyukitanimura kazuyukitanimura kazuyukitanimura approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@huaxingao @codecov-commenter @kazuyukitanimura @hsiang-c @andygrove @parthchandra