feat(flink): add lance format for Flink append only table#18741
feat(flink): add lance format for Flink append only table#18741danny0405 wants to merge 3 commits into
Conversation
hudi-agent
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for working on this! The PR adds Flink Lance support for append-only COW tables and includes both writer/reader plumbing. A few correctness concerns worth double-checking in the inline comments — most notably the order of validation vs. setupConfOptions lets PRIMARY KEY DDL bypass the Lance constraint on the sink path, and a resource leak in the read context when iterator creation fails. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A few naming and simplification suggestions below.
| String baseFileFormat = conf.getString(HoodieTableConfig.BASE_FILE_FORMAT.key(), null); | ||
| if (baseFileFormat != null && HoodieFileFormat.LANCE.name().equalsIgnoreCase(baseFileFormat)) { | ||
| throw new HoodieValidationException(HoodieFileFormat.LANCE_SPARK_ONLY_ERROR_MSG); | ||
| if (conf.containsKey(FlinkOptions.RECORD_KEY_FIELD.key())) { |
There was a problem hiding this comment.
🤖 On the sink path, sanityCheck (and so checkBaseFileFormat) runs before setupConfOptions → setupHoodieKeyOptions, which is where PRIMARY KEY syntax is copied into FlinkOptions.RECORD_KEY_FIELD. So a Lance + operation=insert table declared with PRIMARY KEY (..) NOT ENFORCED passes this validation and only fails on the source side — leaving the user able to write but not read. Would it make sense to also check schema.getPrimaryKey().isPresent() here (and add a test using PRIMARY KEY syntax rather than the option)?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| (HoodieRowDataLanceReader) HoodieIOFactory.getIOFactory(storage) | ||
| .getReaderFactory(HoodieRecord.HoodieRecordType.FLINK) | ||
| .getFileReader(tableConfig, filePath, HoodieFileFormat.LANCE, Option.empty()); | ||
| return rowDataLanceReader.getRowDataIterator(RowDataQueryContexts.fromSchema(requiredSchema).getRowType(), requiredSchema); |
There was a problem hiding this comment.
🤖 If getRowDataIterator(...) throws (e.g. loadNextBatch failure surfaces as HoodieIOException), rowDataLanceReader is leaked — the metadata LanceFileReader and metadataAllocator from the constructor are never released, since the parent reader's close() is only chained through the iterator's close(). Could you wrap this in a try/catch that closes the reader on failure (the CopyOnWriteInputFormat.getLanceRecordIterator path already does this)?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| .mapToObj(i -> DataTypes.FIELD(fullFieldNames[i], fullFieldTypes[i])) | ||
| .toArray(DataTypes.Field[]::new)) | ||
| .bridgedTo(RowData.class); | ||
| HoodieSchema requestedSchema = HoodieSchemaConverter.convertToSchema(selectedDataType.getLogicalType()); |
There was a problem hiding this comment.
🤖 The Lance branch projects only selectedFields from the file and does not inject partition values from the path (which the Parquet branch does via partObjects). If hoodie.datasource.write.drop.partitioncolumns=true, the partition column won't be in the Lance file and orderVectors will throw Missing Lance column in projected batch: <partition> for any query that selects the partition column. Is this combination intended to be unsupported, or should we either inject the value here or block the config in checkBaseFileFormat?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
There was a problem hiding this comment.
not supported yet
| try { | ||
| arrowReader.close(); | ||
| } catch (Exception e) { | ||
| throw new HoodieException("Failed to close Lance Arrow reader", e); |
There was a problem hiding this comment.
🤖 If arrowReader.close() throws, the throw inside the outer finally for lanceReader.close() (or reader.close() inside the innermost finally) will replace it and the original exception is lost — finally-throws override exceptions from the try body. Could you switch to the addSuppressed pattern (e.g. accumulate a primary exception across all four closes)? The Spark equivalent uses log-and-swallow which avoids this entirely.
- AI-generated; verify before applying. React 👍/👎 to flag quality.
There was a problem hiding this comment.
I don't think it is necessary as long as the exception is thrown out.
| batch = arrowReader.getVectorSchemaRoot(); | ||
| orderedVectors = orderVectors(rowType, batch.getFieldVectors()); | ||
| rowId = 0; | ||
| if (batch.getRowCount() == 0) { |
There was a problem hiding this comment.
🤖 Recursing into loadNextBatch() for empty batches will blow the stack if a Lance file contains many consecutive empty batches in a row. Could you convert this to an iterative loop (do { … } while (hasNext && batch.getRowCount() == 0))?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| ArrowType.Decimal decimal = (ArrowType.Decimal) arrowType; | ||
| return new DecimalType(decimal.getPrecision(), decimal.getScale()); | ||
| } else if (arrowType instanceof ArrowType.Timestamp) { | ||
| return new org.apache.flink.table.types.logical.TimestampType(6); |
There was a problem hiding this comment.
🤖 toLogicalType maps every ArrowType.Timestamp to TimestampType(6), even when the Arrow timezone is "UTC" (which the writer uses for TIMESTAMP_WITH_LOCAL_TIME_ZONE). Is that intentional, or should the timezone-set case map to LocalZonedTimestampType(6) so schema round-trips? Today HoodieRowDataLanceReader.getSchema() would silently lose the local-zone-ness.
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| return getFileWriterByFormat(extension, instantTime, storagePath, config, rowType, taskContextSupplier); | ||
| } | ||
|
|
||
| private <T, I, K, O> HoodieFileWriter getFileWriterByFormat( |
There was a problem hiding this comment.
🤖 nit: <T, I, K, O> are declared on this method but none of them appear anywhere in the signature or body — could you drop them?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
There was a problem hiding this comment.
keep it, might need to refactor it in the future.
| /** | ||
| * Builds a Lance write config from storage options carried in the Hadoop configuration. | ||
| */ | ||
| public static HoodieConfig getLanceWriteConfig(org.apache.hadoop.conf.Configuration conf) { |
There was a problem hiding this comment.
🤖 nit: the method body only reads LANCE_READ_* config keys, so getLanceWriteConfig is a misleading name — getLanceReadConfig would match what it actually does.
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| * Validate the base file format. Flink Lance support is scoped to append-only COW tables. | ||
| */ | ||
| private void checkBaseFileFormat(Configuration conf) { | ||
| private void checkBaseFileFormat(Configuration conf, boolean write) { |
There was a problem hiding this comment.
🤖 nit: a bare boolean write makes call sites like checkBaseFileFormat(conf, false) opaque — could you split into two methods (checkBaseFileFormatForRead / checkBaseFileFormatForWrite) or at least name it isWritePath?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| return new HoodieNotSupportedException("Flink Lance base-file support currently supports primitive append-only columns; unsupported type: " + type); | ||
| } | ||
|
|
||
| private static final class LogicalTypeChecks { |
There was a problem hiding this comment.
🤖 nit: this private inner class is just a one-liner forwarding to org.apache.flink.table.types.logical.utils.LogicalTypeChecks.getPrecision — a static import would eliminate the wrapper entirely.
- AI-generated; verify before applying. React 👍/👎 to flag quality.
hudi-agent
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for working through the feedback — the rename to getLanceReadConfig, the split into checkBaseFileFormatForRead/checkBaseFileFormatForWrite, the do/while conversion in loadNextBatch, the iterator-creation cleanup in FlinkRowDataReaderContext, the timezone-aware timestamp mapping (with a round-trip test), and the static-import cleanup all look good. One prior finding still looks unaddressed: the PRIMARY KEY DDL bypass on the sink path in HoodieTableFactory. Please take a look at the inline comment, and this should be ready for a Hudi committer or PMC member to take it from here.
| String baseFileFormat = conf.getString(HoodieTableConfig.BASE_FILE_FORMAT.key(), null); | ||
| if (baseFileFormat != null && HoodieFileFormat.LANCE.name().equalsIgnoreCase(baseFileFormat)) { | ||
| throw new HoodieValidationException(HoodieFileFormat.LANCE_SPARK_ONLY_ERROR_MSG); | ||
| if (conf.containsKey(FlinkOptions.RECORD_KEY_FIELD.key())) { |
There was a problem hiding this comment.
🤖 Re-raising — on the sink path, sanityCheck (line 107) still runs before setupConfOptions (line 108), and checkBaseFileFormatForWrite only looks at conf.containsKey(FlinkOptions.RECORD_KEY_FIELD.key()). A Lance + operation=insert table declared with PRIMARY KEY (..) NOT ENFORCED (no record.key.field option) will still pass this validation and only fail on read. Could you also check schema.getPrimaryKey().isPresent() here (e.g. by passing the schema through) and add a test using SchemaBuilder.primaryKey(...) rather than just the option?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
Describe the issue this Pull Request addresses
Flink currently does not have an append-only Lance base-file path for tables without primary keys. Lance support needs Flink-specific RowData writer and reader plumbing, table-format validation, and COW input-format handling so pk-less append-only tables can ingest and read Lance base files.
This PR adds Lance support for Flink COPY_ON_WRITE append-only INSERT tables without primary keys. It explicitly rejects unsupported Lance combinations such as primary-key tables, MERGE_ON_READ tables, and non-INSERT write operations. This PR introduces user-visible storage-format support for Flink under those constraints.
Summary and Changelog
Flink can now write and read Lance base files for append-only COW tables without primary keys, including projected reads from Lance files.
Working tree: Add Flink Lance append-only writer/reader support
HoodieRowDataLanceWriter,HoodieFlinkLanceArrowUtils, andHoodieBloomFilterStringWriteSupportfor primitive RowData-to-Arrow Lance writes.HoodieRowDataCreateHandleandHoodieRowDataFileWriterFactoryto dispatch writers by base-file extension and create Lance writers.HoodieRowDataLanceReaderand wired it throughHoodieRowDataFileReaderFactory,FlinkRowDataReaderContext, andCopyOnWriteInputFormat.select name, uuidreturn columns in Flink projection order.CopyOnWriteInputFormat.StreamerUtil.getLanceWriteConfig(...)and persistedhoodie.table.base.file.formatduring table initialization.HoodieTableFactoryvalidation to allow Lance only for append-only COPY_ON_WRITE INSERT tables without primary keys.ITTestHoodieDataSource#testLanceFormatAppendOnlyWriteAndRead.Impact
This enables a new Flink write/read path for Lance base files, scoped to COPY_ON_WRITE append-only tables without primary keys. Existing Parquet behavior is preserved, with writer creation now dispatched by file extension instead of always creating a Parquet writer.
Unsupported Lance table shapes fail early with explicit validation messages. Complex/nested logical types are not supported by the Flink Lance Arrow conversion helpers in this change; unsupported types throw
HoodieNotSupportedException.Risk Level
medium
This touches storage-format writer/reader paths, Flink table validation, COW input-format reading, and table initialization. The main risks are projection correctness, resource cleanup, and accidental enablement for unsupported table modes. Mitigation includes focused compile and integration/unit coverage:
mvn -pl hudi-flink-datasource/hudi-flink -am -DskipTests -DskipITs -Dscala-2.12 compilemvn -pl hudi-flink-datasource/hudi-flink -am -Dscala-2.12 -Dtest=TestHoodieTableFactory#testLanceFormatSupportedForAppendOnlyTables,org.apache.hudi.table.catalog.TestHoodieCatalog#testCreateAppendOnlyLanceTableWithoutPrimaryKey,ITTestHoodieDataSource#testLanceFormatAppendOnlyWriteAndRead -Dsurefire.failIfNoSpecifiedTests=false testThe focused test run passed with
Tests run: 3, Failures: 0, Errors: 0.Documentation Update
Documentation update is recommended because this adds new user-facing Flink Lance support with important constraints: only COPY_ON_WRITE append-only INSERT tables without primary keys are supported, and primitive columns are supported by the current RowData/Arrow conversion path.
Contributor's checklist