-
Notifications
You must be signed in to change notification settings - Fork 57
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
SNOW-1748333 Fix Iceberg decimal type schema parser #864
Conversation
src/main/java/net/snowflake/ingest/streaming/internal/ParquetRowBuffer.java
Show resolved
Hide resolved
src/main/java/net/snowflake/ingest/utils/IcebergDataTypeParser.java
Outdated
Show resolved
Hide resolved
src/test/java/net/snowflake/ingest/streaming/internal/datatypes/IcebergNumericTypesIT.java
Outdated
Show resolved
Hide resolved
} | ||
list.add( | ||
new BigDecimal( | ||
RandomStringUtils.randomNumeric(intPart) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes it impossible to reproduce failures locally. Why do we need this randomNumeric when we already generated a random number for intpart and floatpart using the seeded RNG in lines 412 and 413?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The length of decimal is greater than int/long.max, thus we need string generator for this. Switch to generator with consistent seed.
f0286a1
to
7787aa5
Compare
src/test/java/net/snowflake/ingest/streaming/internal/datatypes/IcebergNumericTypesIT.java
Outdated
Show resolved
Hide resolved
bigDecimals_38_10); | ||
} | ||
|
||
private static List<Object> randomBigDecimal(int count, int precision, int scale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure i fully understand what this is trying to do (comments will help?). Lets get this merged in to unblock KC folks and I'll follow up with you offline on this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, test code we'll discuss after this gets merged.
Summary
This PR addresses the issue where the scanner encounters an error when scanning delta-encoded decimal values with a precision greater than 18.
Changes
sfVer
metadata in Iceberg mode: This prevents the scanner from applying optimizations specific to Snowflake-written Parquet files, which was the root cause of the issue with scanning.