Skip to content

GH-3710: Tolerate unrecognized logical/physical type combinations when reading #3711

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

Open
divjotarora wants to merge 4 commits into
base: master
Choose a base branch
from
Open

GH-3710: Tolerate unrecognized logical/physical type combinations when reading #3711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8469645
Tolerate unrecognized logical/physical type combinations when reading
divjotarora Aug 6, 2026
7f4a281
address comments + test
divjotarora Aug 18, 2026
ec65103
address comments
divjotarora Aug 20, 2026
0e17db9
update parquet-testing file ref
divjotarora Sep 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 53 additions & 5 deletions parquet-column/src/main/java/org/apache/parquet/schema/Types.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,15 @@
public class Types {
private static final int NOT_SET = 0;

/**
* Thrown when a logical type annotation is not applicable to a column's physical type.
*/
public static class UnsupportedLogicalTypeAnnotation extends IllegalStateException {
public UnsupportedLogicalTypeAnnotation(String message) {
super(message);
}
}

/**
* A base builder for {@link Type} objects.
*
Expand Down Expand Up @@ -344,6 +353,9 @@ public abstract static class BasePrimitiveBuilder<P, THIS extends BasePrimitiveB
private int precision = NOT_SET;
private int scale = NOT_SET;
private ColumnOrder columnOrder;
// When true and an unsupported logical/physical type combination is encountered, the
// annotation is dropped and the column order is forced to "undefined" so stats are ignored.

@RussellSpitzer RussellSpitzer Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a load bearing comment :) I would probably skip all this since it's a implementation detail that can go stale pretty quickly. Instead stick to the behavior.

For example you could say "ignoreUnsupportedLogicalAnnotations" as the variable name and I think that would cover it? Or "dropUnsupportedLogicalAnnotations"?

private boolean dropUnsupportedLogicalTypeCombinations = false;

private BasePrimitiveBuilder(P parent, PrimitiveTypeName type) {
super(parent);
Expand Down Expand Up @@ -426,8 +438,38 @@ public THIS columnOrder(ColumnOrder columnOrder) {
return self();
}

/**
* When set, an unsupported combination results in the logical type annotation being dropped
* rather than throwing. The associated statistics are also forcefully ignored by setting the
* column order to {@link ColumnOrderName#UNDEFINED}.
*
* @return this builder for method chaining
*/
public THIS dropUnsupportedLogicalTypeCombinations() {
Comment thread
divjotarora marked this conversation as resolved.
this.dropUnsupportedLogicalTypeCombinations = true;
return self();
}

@Override
protected PrimitiveType build(String name) {
try {
return validateAndBuild(name);
} catch (UnsupportedLogicalTypeAnnotation e) {
if (!dropUnsupportedLogicalTypeCombinations) {
throw e;
}

LOGGER.warn(
"Dropping unsupported logical type annotation {} on physical type {}: {}",
logicalTypeAnnotation,
primitiveType,
e.getMessage());
return new PrimitiveType(
repetition, primitiveType, length, name, null, null, id, ColumnOrder.undefined());
}
}

private PrimitiveType validateAndBuild(String name) {
if (length == 0 && logicalTypeAnnotation instanceof LogicalTypeAnnotation.UUIDLogicalTypeAnnotation) {
length = LogicalTypeAnnotation.UUIDLogicalTypeAnnotation.BYTES;
}
Expand Down Expand Up @@ -590,9 +632,15 @@ public Optional<Boolean> visit(
return checkBinaryPrimitiveType(geographyLogicalType);
}

private void checkAnnotation(boolean valid, String message, Object... args) {

@RussellSpitzer RussellSpitzer Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd generally want to avoid passing a "boolean" into the "checkAnnotation" method. Since it doesn't actually check an annotation here, it just throws an exception if you pass through is false. I think this is a bit over-indexing on the previous "checkState" work and possibly hiding some other holes.

I would probably try to re-think the whole visitor here rather than try to wrap a previous failure differently.

Instead of what we have here maybe something like

AllowedPhysicalTypes visitor

/**
 * The physical types a logical type annotation may annotate. An empty set means the annotation
 * cannot be applied to a primitive type at all.
 */
private static final class AllowedPhysicalTypes {
  private static final AllowedPhysicalTypes NONE =
      new AllowedPhysicalTypes(EnumSet.noneOf(PrimitiveTypeName.class), NOT_SET);
  private static final AllowedPhysicalTypes ANY =
      new AllowedPhysicalTypes(EnumSet.allOf(PrimitiveTypeName.class), NOT_SET);
  private final Set<PrimitiveTypeName> types;
  private final int requiredLength;
  static Optional<AllowedPhysicalTypes> of(PrimitiveTypeName... types) {
    return Optional.of(new AllowedPhysicalTypes(EnumSet.copyOf(asList(types)), NOT_SET));
  }
  static Optional<AllowedPhysicalTypes> fixed(int requiredLength) {
    return Optional.of(new AllowedPhysicalTypes(
        EnumSet.of(PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY), requiredLength));
  }
  boolean accepts(PrimitiveTypeName type, int length) {
    return types.contains(type) && (requiredLength == NOT_SET || length == requiredLength);
  }
  @Override
  public String toString() {
    if (requiredLength != NOT_SET) {
      return PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY + "(" + requiredLength + ")";
    }
    return types.stream().map(Enum::name).collect(Collectors.joining(", "));
  }
}
private static final LogicalTypeAnnotationVisitor<AllowedPhysicalTypes> ALLOWED_PHYSICAL_TYPES =
    new LogicalTypeAnnotationVisitor<AllowedPhysicalTypes>() {
      @Override public Optional<AllowedPhysicalTypes> visit(StringLogicalTypeAnnotation t) { return of(BINARY); }
      @Override public Optional<AllowedPhysicalTypes> visit(JsonLogicalTypeAnnotation t) { return of(BINARY); }
      @Override public Optional<AllowedPhysicalTypes> visit(BsonLogicalTypeAnnotation t) { return of(BINARY); }
      @Override public Optional<AllowedPhysicalTypes> visit(EnumLogicalTypeAnnotation t) { return of(BINARY); }
      @Override public Optional<AllowedPhysicalTypes> visit(GeometryLogicalTypeAnnotation t) { return of(BINARY); }
      @Override public Optional<AllowedPhysicalTypes> visit(GeographyLogicalTypeAnnotation t) { return of(BINARY); }
      @Override public Optional<AllowedPhysicalTypes> visit(DateLogicalTypeAnnotation t) { return of(INT32); }
      @Override public Optional<AllowedPhysicalTypes> visit(TimestampLogicalTypeAnnotation t) { return of(INT64); }
      @Override public Optional<AllowedPhysicalTypes> visit(UUIDLogicalTypeAnnotation t) { return fixed(UUIDLogicalTypeAnnotation.BYTES); }
      @Override public Optional<AllowedPhysicalTypes> visit(Float16LogicalTypeAnnotation t) { return fixed(Float16LogicalTypeAnnotation.BYTES); }
      @Override public Optional<AllowedPhysicalTypes> visit(IntervalLogicalTypeAnnotation t) { return fixed(12); }
      @Override public Optional<AllowedPhysicalTypes> visit(TimeLogicalTypeAnnotation t) {
        return t.getUnit() == TimeUnit.MILLIS ? of(INT32) : of(INT64);
      }
      @Override public Optional<AllowedPhysicalTypes> visit(IntLogicalTypeAnnotation t) {
        return t.getBitWidth() == 64 ? of(INT64) : of(INT32);
      }
      @Override public Optional<AllowedPhysicalTypes> visit(DecimalLogicalTypeAnnotation t) {
        return of(INT32, INT64, BINARY, FIXED_LEN_BYTE_ARRAY);
      }
      @Override public Optional<AllowedPhysicalTypes> visit(UnknownLogicalTypeAnnotation t) {
        return Optional.of(ANY);
      }
    };

Then instead of trying to throw our way to control flow we can do something like

if (logicalTypeAnnotation != null) {
  AllowedPhysicalTypes allowed =
      logicalTypeAnnotation.accept(ALLOWED_PHYSICAL_TYPES).orElse(AllowedPhysicalTypes.NONE);
  if (!allowed.accepts(primitiveType, length)) {
    if (!dropUnsupportedLogicalAnnotations) {
      throw new IllegalStateException(allowed.isEmpty()
          ? logicalTypeAnnotation + " can not be applied to a primitive type"
          : String.format("%s can only annotate %s", logicalTypeAnnotation, allowed));
    }
    LOGGER.warn(
        "Dropping unsupported logical type annotation {} on physical type {}",
        logicalTypeAnnotation, primitiveType);
    return new PrimitiveType(
        repetition, primitiveType, this.length, name, null, null, id, ColumnOrder.undefined());
  }
  validateDecimalPrecision(meta);
}

if (!valid) {
throw new UnsupportedLogicalTypeAnnotation(String.format(message, args));
}
}

private Optional<Boolean> checkFixedPrimitiveType(
int l, LogicalTypeAnnotation logicalTypeAnnotation) {
Preconditions.checkState(
checkAnnotation(
primitiveType == PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY && length == l,
"%s can only annotate FIXED_LEN_BYTE_ARRAY(%s)",
logicalTypeAnnotation,
Expand All @@ -602,7 +650,7 @@ private Optional<Boolean> checkFixedPrimitiveType(

private Optional<Boolean> checkBinaryPrimitiveType(
LogicalTypeAnnotation logicalTypeAnnotation) {
Preconditions.checkState(
checkAnnotation(
primitiveType == PrimitiveTypeName.BINARY,
"%s can only annotate BINARY",
logicalTypeAnnotation);
Expand All @@ -611,7 +659,7 @@ private Optional<Boolean> checkBinaryPrimitiveType(

private Optional<Boolean> checkInt32PrimitiveType(
LogicalTypeAnnotation logicalTypeAnnotation) {
Preconditions.checkState(
checkAnnotation(
primitiveType == PrimitiveTypeName.INT32,
"%s can only annotate INT32",
logicalTypeAnnotation);
Expand All @@ -620,14 +668,14 @@ private Optional<Boolean> checkInt32PrimitiveType(

private Optional<Boolean> checkInt64PrimitiveType(
LogicalTypeAnnotation logicalTypeAnnotation) {
Preconditions.checkState(
checkAnnotation(
primitiveType == PrimitiveTypeName.INT64,
"%s can only annotate INT64",
logicalTypeAnnotation);
return Optional.of(true);
}
})
.orElseThrow(() -> new IllegalStateException(
.orElseThrow(() -> new UnsupportedLogicalTypeAnnotation(
logicalTypeAnnotation + " can not be applied to a primitive type"));
}

Expand Down
13 changes: 13 additions & 0 deletions parquet-column/src/test/java/org/apache/parquet/schema/TestTypeBuilders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1605,4 +1605,17 @@ public void testGeographyLogicalTypeWithoutEdgeInterpolationAlgorithm() {
Types.optional(BINARY).as(LogicalTypeAnnotation.geographyType()).named("aGeography");
assertThat(optionalGeographyActual).isEqualTo(optionalGeographyExpected);
}

@Test
public void testDropUnsupportedLogicalTypeCombinations() {
// Other tests already validate that unsupported type combinations throw by default, so this

@RussellSpitzer RussellSpitzer Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd probably drop this description?

// test only validates that the dropUnsupportedLogicalTypeCombinations flag works.
PrimitiveType pt = Types.required(BOOLEAN)
.dropUnsupportedLogicalTypeCombinations()
.as(LogicalTypeAnnotation.timestampType(true, MILLIS))
.named("bool_ts");
assertThat(pt.getPrimitiveTypeName()).isEqualTo(BOOLEAN);
assertThat(pt.getLogicalTypeAnnotation()).isNull(); // Dropped
assertThat(pt.columnOrder().getColumnOrderName()).isEqualTo(ColumnOrder.ColumnOrderName.UNDEFINED);
}
}
2 changes: 2 additions & 0 deletions ...et-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2062,6 +2062,8 @@ private void buildChildren(
}
primitiveBuilder.columnOrder(columnOrder);
}
// Gracefully handle unsupported logical type combinations on the read path.
primitiveBuilder.dropUnsupportedLogicalTypeCombinations();
childBuilder = primitiveBuilder;
} else {
childBuilder = builder.group(fromParquetRepetition(schemaElement.repetition_type));
Expand Down
62 changes: 62 additions & 0 deletions ...adoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,16 @@
import org.apache.parquet.format.GeospatialStatistics;
import org.apache.parquet.format.LogicalType;
import org.apache.parquet.format.MapType;
import org.apache.parquet.format.MilliSeconds;
import org.apache.parquet.format.PageHeader;
import org.apache.parquet.format.PageType;
import org.apache.parquet.format.RowGroup;
import org.apache.parquet.format.SchemaElement;
import org.apache.parquet.format.StringType;
import org.apache.parquet.format.TimeUnit;
import org.apache.parquet.format.TimestampType;
import org.apache.parquet.format.Type;
import org.apache.parquet.format.TypeDefinedOrder;
import org.apache.parquet.format.Util;
import org.apache.parquet.hadoop.ParquetReader;
import org.apache.parquet.hadoop.ParquetWriter;
Expand Down Expand Up @@ -2279,4 +2283,62 @@ public void testColumnIndexNanCountsRoundTrip() {
assertThat(roundTrip).isNotNull();
assertThat(roundTrip.getNanCounts()).containsExactly(1L, 0L, 0L);
}

@Test
Comment thread
divjotarora marked this conversation as resolved.
public void testUnsupportedTypeCombinationDropsAnnotationAndStats() {
ParquetMetadataConverter converter = new ParquetMetadataConverter();
TimeUnit unit = new TimeUnit();
unit.setMILLIS(new MilliSeconds());
SchemaElement leaf = new SchemaElement("bool_ts")
.setRepetition_type(FieldRepetitionType.OPTIONAL)
.setType(Type.BOOLEAN)
.setLogicalType(LogicalType.TIMESTAMP(new TimestampType(true, unit)));
List<SchemaElement> parquetSchema = Lists.newArrayList(new SchemaElement("Message").setNum_children(1), leaf);
List<org.apache.parquet.format.ColumnOrder> columnOrders =
Lists.newArrayList(new org.apache.parquet.format.ColumnOrder());
columnOrders.get(0).setTYPE_ORDER(new TypeDefinedOrder());

MessageType schema = converter.fromParquetSchema(parquetSchema, columnOrders);

PrimitiveType result = schema.getType("bool_ts").asPrimitiveType();
assertThat(result.getPrimitiveTypeName()).isEqualTo(PrimitiveTypeName.BOOLEAN);
assertThat(result.getLogicalTypeAnnotation()).isNull();
assertThat(result.columnOrder().getColumnOrderName()).isEqualTo(ColumnOrder.ColumnOrderName.UNDEFINED);
}

private static PrimitiveType droppedAnnotationInt32() {
return Types.optional(PrimitiveTypeName.INT32)
.columnOrder(ColumnOrder.undefined())
.named("ts_int32");
}

@Test
public void testDroppedAnnotationIgnoresStats() {
ParquetMetadataConverter converter = new ParquetMetadataConverter();
org.apache.parquet.format.Statistics stats = new org.apache.parquet.format.Statistics();
stats.setMin_value(new byte[] {1, 2, 3, 4});
stats.setMax_value(new byte[] {0, 1, 2, 3});
stats.setNull_count(3L);

Statistics<?> result = converter.fromParquetStatistics(Version.FULL_VERSION, stats, droppedAnnotationInt32());

assertThat(result.hasNonNullValue()).isFalse();
assertThat(result.isNumNullsSet()).isTrue();
assertThat(result.getNumNulls()).isEqualTo(3L);
}

@Test
public void testDroppedAnnotationColumnIndexIsNull() {
PrimitiveType int32Type = Types.required(PrimitiveTypeName.INT32).named("i32");
ColumnIndexBuilder cb = ColumnIndexBuilder.getBuilder(int32Type, Integer.MAX_VALUE);
Statistics<?> stats = Statistics.createStats(int32Type);
stats.updateStats(-100);
stats.updateStats(100);
cb.add(stats, null);
org.apache.parquet.format.ColumnIndex parquetColumnIndex =
ParquetMetadataConverter.toParquetColumnIndex(int32Type, cb.build());

assertThat(ParquetMetadataConverter.fromParquetColumnIndex(droppedAnnotationInt32(), parquetColumnIndex))
.isNull();
}
}
71 changes: 71 additions & 0 deletions parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestReadInvalidTypeCombination.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.parquet.hadoop;

import static org.assertj.core.api.Assertions.assertThat;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.Path;
import org.apache.parquet.example.data.Group;
import org.apache.parquet.hadoop.example.GroupReadSupport;
import org.apache.parquet.hadoop.metadata.ParquetMetadata;
import org.apache.parquet.hadoop.util.HadoopInputFile;
import org.apache.parquet.schema.ColumnOrder.ColumnOrderName;
import org.apache.parquet.schema.PrimitiveType;
import org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName;
import org.junit.jupiter.api.Test;

public class TestReadInvalidTypeCombination {

// parquet-testing file with an invalid logical/physical type combination.
private static final String REFERENCE_FILE = "int32_with_uuid_logical_type.parquet";
private static final String REFERENCE_CHANGESET = "4b1ce4502afff8d20c9b4bb08d07e04e21cdeff3";

private final InterOpTester interop = new InterOpTester();

@Test
public void testReadInvalidTypeCombinationSucceeds() throws Exception {
Configuration conf = new Configuration();
Path file = interop.GetInterOpFile(REFERENCE_FILE, REFERENCE_CHANGESET);

// The footer parse should succeed and drop the annotation and stats for the column.
try (ParquetFileReader reader = ParquetFileReader.open(HadoopInputFile.fromPath(file, conf))) {
ParquetMetadata footer = reader.getFooter();
PrimitiveType column =
footer.getFileMetaData().getSchema().getType("int32_uuid").asPrimitiveType();

assertThat(column.getPrimitiveTypeName()).isEqualTo(PrimitiveTypeName.INT32);
assertThat(column.getLogicalTypeAnnotation()).isNull();
assertThat(column.columnOrder().getColumnOrderName()).isEqualTo(ColumnOrderName.UNDEFINED);
}

// The physical values are still fully readable.
int rows = 0;
try (ParquetReader<Group> reader = ParquetReader.builder(new GroupReadSupport(), file)
.withConf(conf)
.build()) {
Group g;
while ((g = reader.read()) != null) {
assertThat(g.getInteger("int32_uuid", 0)).isEqualTo(rows);
rows++;
}
}
assertThat(rows).isEqualTo(10);
}
}