Skip to content

GH-3877: Add FINITE_VECTOR logical type support - #3876

Open
divjotarora wants to merge 15 commits into
apache:masterfrom
divjotarora:vector
Open

divjotarora wants to merge 15 commits into
apache:masterfrom
divjotarora:vector

Conversation

@divjotarora

@divjotarora divjotarora commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Rationale for this change

The FINITE_VECTOR proposal defines fixed-size vectors of non-null, finite elements on canonical LIST storage, so applications can express vector semantics while retaining existing Parquet storage and interoperability. This change brings parquet-java support in line with the proposal.

What changes are included in this PR?

Adds FINITE_VECTOR schema annotations and mandatory write validation, validates stored levels and elements during reads, and preserves LIST interoperability across existing model readers.

Are these changes tested?

The affected Maven suites for FINITE_VECTOR schemas, writing, reading, adapters, and filtered reads passed, and Spotless passed for all touched modules.

Closes #3877

@divjotarora
divjotarora marked this pull request as ready for review October 22, 2026 10:39
@divjotarora divjotarora changed the title MINOR: Add VECTOR logical type support GH-3877: Add VECTOR logical type support Oct 22, 2026
@divjotarora divjotarora changed the title GH-3877: Add VECTOR logical type support GH-3877: Add FINITE_VECTOR logical type support Nov 2, 2026

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The biggest issue is around the validation. It feels to me that a lot of visitor that are meant to do other things, are now also validating if it is a valid FINITE_VECTOR. I left some other comments as well. Let me know what you think

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Eventually, these should go in https://github.com/apache/parquet-testing/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's definitely the goal here

@divjotarora
divjotarora requested a review from Fokko November 4, 2026 15:36

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @divjotarora for iterating on this, I think we're getting close 👍 Now, my main concern is the performance penalty that we're having because of the validation.

RecordConsumer recordWriter = new MessageColumnIORecordConsumer(columns);
if (DEBUG) recordWriter = new RecordConsumerLoggingWrapper(recordWriter);
return validating ? new ValidatingRecordConsumer(recordWriter, getType()) : recordWriter;
return validating || TypeUtil.hasFiniteVector(getType())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on the discussion on the dev-list, we could also drop the validation. One of the goals of the VECTOR is performance, so I think we can also delegate this to the engine.

Comment on lines +1594 to +1596
default Optional<T> visit(FiniteVectorLogicalTypeAnnotation vectorLogicalType) {
return empty();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can do the fallback here:

Suggested change
default Optional<T> visit(FiniteVectorLogicalTypeAnnotation vectorLogicalType) {
return empty();
}
default Optional<T> visit(FiniteVectorLogicalTypeAnnotation vectorLogicalType) {
return visit((ListLogicalTypeAnnotation) vectorLogicalType);
}

Comment on lines +539 to +543
@Override
public OriginalType toOriginalType() {
return OriginalType.LIST;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this is inherited from ListLogicalTypeAnnotation, so we can drop it:

Suggested change
@Override
public OriginalType toOriginalType() {
return OriginalType.LIST;
}

Less is more 👍

? new GroupType(repetition, name, logicalTypeAnnotation, fields, id)
: new GroupType(repetition, name, getOriginalType(), fields, id);
if (logicalTypeAnnotation instanceof FiniteVectorLogicalTypeAnnotation) {
checkFiniteVector(group);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for moving this into the builder! There's still a public way around it: GroupType.withNewFields(...) keeps the annotation and calls the constructor directly. I tried it: a FINITE_VECTOR group with an optional float element (via withNewFields) writes fine, and then ParquetFileReader.open on that same file fails with Invalid FINITE_VECTOR schema in file metadata. So we can produce files we can't read ourselves.

Can we also call checkFiniteVector from TypeUtil.checkValidWriteSchema? ParquetFileWriter already calls that for every write, so it closes the gap without touching the GroupType constructors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ack, I think that's what I was missing earlier and why I'd put all of this in the GroupType constructors before. Your suggestion makes sense, I'll look into it.

buildChildren(builder, iterator, root.getNum_children(), columnOrderIterator);
return builder.named(root.name);
} catch (InvalidSchemaException e) {
throw new ParquetDecodingException("Invalid FINITE_VECTOR schema in file metadata", e);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This wraps every InvalidSchemaException from the builders with a FINITE_VECTOR-specific message. That's true today, but as soon as another type adds builder validation the message is misleading. Could we keep the original message, or only rethrow with this message when the cause came from the vector check?

private void checkVectorColumnCounts(ColumnChunkMetaData column, long rows) {
MessageType fileSchema = getFileMetaData().getSchema();
String[] path = column.getPath().toArray();
FiniteVectorLogicalTypeAnnotation vector = TypeUtil.getFiniteVectorType(fileSchema, path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small perf thing: this walks the schema (getFiniteVectorType + getColumnDescription) for every column chunk of every row group, and again in readAllPages. We could compute a Map<ColumnPath, Integer> of vector lengths once when the reader is constructed; for files without vectors that's a single isEmpty() check.

do {
ColumnReader columnReader = currentState.column;
int d = columnReader.getCurrentDefinitionLevel();
currentState.validateVector(d);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This validates every vector value on the record read path, which costs ~15–25% on vector-heavy reads in my benchmark. Based on the point raised on the dev-list, I'd remove this check for now. C++ does not validate this either.

if (value.length() != 2) {
throw invalidVector("invalid FLOAT16 element length: " + value.length());
}
if ((value.get2BytesLittleEndian() & 0x7c00) == 0x7c00) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same FLOAT16 bit check is also in ValidatingRecordConsumer#addBinary. How about adding Float16.isFinite(short) next to the existing Float16.isNaN(short) and using that in both places?

More broadly, this class is already the state machine for "a valid vector level stream". If the writer also checks at the column level (see my comment on MessageColumnIO), both sides can share it, and the vector logic in ValidatingRecordConsumer can go.

writer changes need not produce byte-identical files.

```bash
JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 /home/div.arora/.m2/wrapper/dists/apache-maven-3.9.16/c51baac8/bin/mvn -Dthrift.executable=/home/div.arora/parquet-java-worktrees/vector/target/vector-tools/bin/thrift -pl parquet-hadoop -am -DskipTests package dependency:build-classpath -DincludeScope=test -Dmdep.outputFile=target/vector-test-classpath.txt -Dmdep.regenerateFile=true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This contains local paths (/home/..., a local Thrift binary) and some review history (pre-rename notes, "Round 2" generator). Could we trim it down to what the files contain and how to regenerate them with the repo's standard build? Once the files move to parquet-testing, this can go there too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I plan to delete all of this and move only the Parquet files + a different cleaned up README to parquet-testing. This is primarily tracking for me (and the agents 😅 ) right now.

@Fokko

Fokko commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Another thing, I think we don't need to collect that much statistics for the vector:

  • min/max don't add much value
  • Bloom filters even less
  • null-count does not make much sense, since the fields are required

We can also do this in a separate PR

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add VECTOR logical type reference implementation

2 participants