GH-3142: Add strict validation for unsigned types in ExampleParquetWriter #3271

ArnavBalyan · 2025-08-19T07:34:01Z

Summary:

This PR addresses the issue of ExampleParquetWriter writing invalid values for unsigned integer types.
Updated the ExampleParquetWriter.Builder with optional withStrictUnsignedIntegerValidation which can be enabled by caller.
Fixes ExampleParquetWriter may write illegal values for uint_8 and uint_16 #3142

Testing:

Added UTs

parthchandra · 2025-08-19T17:53:32Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/example/ExampleParquetWriter.java


  public static class Builder extends ParquetWriter.Builder<Group, Builder> {
    private MessageType type = null;
+    private boolean strictUnsignedIntegerValidation = false;


I feel this should be true always. At least should be true by default.

Sure will will update if there are no other changes requested, cc @shangxinli @wgtmac could you please review thanks!

Updated this default value to true thanks!

shangxinli · 2025-08-25T15:44:36Z

...oop/src/test/java/org/apache/parquet/hadoop/example/TestStrictUnsignedIntegerValidation.java

+/**
+ * Integration test for strict unsigned integer validation in ExampleParquetWriter.
+ */
+public class TestStrictUnsignedIntegerValidation {


The test cases cover all unsigned types
and both valid and invalid scenarios. Consider adding a test case for
the maximum values of each unsigned type (255 for UINT_8, 65535 for
UINT_16, etc.) to ensure boundary conditions are properly handled.

Updated thanks!

nandorKollar · 2025-08-27T21:03:24Z

...src/main/java/org/apache/parquet/hadoop/example/ValidatingUnsignedIntegerRecordConsumer.java

+    Type currentType = getCurrentFieldType();
+    if (currentType != null && currentType.isPrimitive()) {
+      LogicalTypeAnnotation logicalType = currentType.asPrimitiveType().getLogicalTypeAnnotation();
+      if (logicalType instanceof LogicalTypeAnnotation.IntLogicalTypeAnnotation) {


Consider using LogicalTypeAnnotationVisitor here and in validateUnsignedLong method, instead of instanceof.

Updated if you could PTAL

wgtmac · 2025-08-28T02:04:25Z

...src/main/java/org/apache/parquet/hadoop/example/ValidatingUnsignedIntegerRecordConsumer.java

+ * to unsigned integer fields (UINT_8, UINT_16, UINT_32, UINT_64) are within
+ * the valid unsigned range before delegating to the wrapped consumer.
+ */
+public class ValidatingUnsignedIntegerRecordConsumer extends RecordConsumer {


I'm not sure if it is enough to modify lines below to add the check:

https://github.com/apache/parquet-java/blob/master/parquet-column/src/main/java/org/apache/parquet/io/ValidatingRecordConsumer.java#L203-L206

WDYT? @parthchandra

Hmm, that's a good idea. Keep in mind that we will get the validation in any place where a writer is created withValidation, not just in the example. I'm in favor of it though.

Yes, that is consistent with other validations. I just think that adding a new class for unsigned integer is an overkill, especially when we already have a ValidatingRecordConsumer.java that serves the same purpose.

Yes sure, will update to piggyback on ValidatingRecordConsumer

ArnavBalyan · 2025-08-29T04:17:27Z

cc @parthchandra @wgtmac could you PTAL thanks

wgtmac · 2025-08-29T16:21:01Z

parquet-column/src/main/java/org/apache/parquet/io/ValidatingRecordConsumer.java

+  private static final int UINT_16_MAX_VALUE = 65535;
+
  private final RecordConsumer delegate;
+  private final boolean strictUnsignedIntegerValidation;


Shouldn't it be validated at all times? This is already the ValidatingRecordConsumer so it might need to validate everything.

Unfortunately there are several test breaking and existing code built with the assumption of no validation. Since we put the changes directly within ValidatingRecordConsumer it causes existing behaviour to break. I added this with a feature flag so new users can start using the validation. I'll investigate as a followup if we can cleanup/fix existing code thanks!

Could you help investigate why existing test break? Are all of them failed because of unsigned integer overflow? If yes, they are actually bugs and should be fixed in this PR so we don't have to add a separate flag for unsigned integer.

wgtmac · 2025-08-29T16:21:16Z

parquet-column/src/main/java/org/apache/parquet/io/ValidatingRecordConsumer.java

  private static final Logger LOG = LoggerFactory.getLogger(ValidatingRecordConsumer.class);

+  private static final int UINT_8_MAX_VALUE = 255;
+  private static final int UINT_16_MAX_VALUE = 65535;


Why only 8 and 16?

UINT_8, UINT_16, and UINT_32 annotate an int32 value, UINT_64 annotatyes a i64 value. For UINT_8, and UINT16 valid values are in the range [0, UINT_8_MAX_VALUE] and [0, UINT_16_MAX_VALUE] respectively. For UINT_32 and UINT_64, all non negative values are valid.

yes that was the reason thanks!

parthchandra

lgtm

parthchandra · 2025-08-29T18:16:06Z

parquet-column/src/main/java/org/apache/parquet/io/ValidatingRecordConsumer.java

  private static final Logger LOG = LoggerFactory.getLogger(ValidatingRecordConsumer.class);

+  private static final int UINT_8_MAX_VALUE = 255;
+  private static final int UINT_16_MAX_VALUE = 65535;


UINT_8, UINT_16, and UINT_32 annotate an int32 value, UINT_64 annotatyes a i64 value. For UINT_8, and UINT16 valid values are in the range [0, UINT_8_MAX_VALUE] and [0, UINT_16_MAX_VALUE] respectively. For UINT_32 and UINT_64, all non negative values are valid.

parthchandra · 2025-08-29T18:16:51Z

parquet-column/src/main/java/org/apache/parquet/io/ValidatingRecordConsumer.java

+  private static final int UINT_16_MAX_VALUE = 65535;
+
  private final RecordConsumer delegate;
+  private final boolean strictUnsignedIntegerValidation;


ArnavBalyan · 2025-08-30T04:31:56Z

cc @wgtmac this is ready thanks!

ArnavBalyan mentioned this pull request Aug 19, 2025

ExampleParquetWriter may write illegal values for uint_8 and uint_16 #3142

Open

parthchandra reviewed Aug 19, 2025

View reviewed changes

shangxinli reviewed Aug 25, 2025

View reviewed changes

nandorKollar reviewed Aug 27, 2025

View reviewed changes

wgtmac reviewed Aug 28, 2025

View reviewed changes

ArnavBalyan force-pushed the arnavb/test-pqt branch from aeb8b4f to 566a374 Compare August 29, 2025 04:17

ArnavBalyan force-pushed the arnavb/test-pqt branch 2 times, most recently from d8cc9ca to 6beeef3 Compare August 29, 2025 08:01

ArnavBalyan requested a review from parthchandra August 29, 2025 08:04

wgtmac reviewed Aug 29, 2025

View reviewed changes

parthchandra reviewed Aug 29, 2025

View reviewed changes

ArnavBalyan added 3 commits August 29, 2025 19:21

update

d18c2ee

lint

b774c19

update

f3cfe41

ArnavBalyan force-pushed the arnavb/test-pqt branch from 6beeef3 to f3cfe41 Compare August 29, 2025 19:21

GH-3142: Add strict validation for unsigned types in ExampleParquetWriter #3271

Are you sure you want to change the base?

GH-3142: Add strict validation for unsigned types in ExampleParquetWriter #3271

Uh oh!

Conversation

ArnavBalyan commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArnavBalyan Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArnavBalyan commented Aug 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthchandra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArnavBalyan commented Aug 30, 2025

Uh oh!

Uh oh!

ArnavBalyan commented Aug 19, 2025 •

edited

Loading

ArnavBalyan Aug 29, 2025 •

edited

Loading