hudi-agent commented on code in PR #19283:
URL: https://github.com/apache/hudi/pull/19283#discussion_r3585634467
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/compact/HoodieCompactor.java:
##########
@@ -174,8 +174,7 @@ public List<WriteStatus> logCompact(HoodieWriteConfig
writeConfig,
TaskContextSupplier taskContextSupplier)
throws IOException {
HoodieReaderContext<IndexedRecord> readerContext = new
HoodieAvroReaderContext(
table.getStorageConf(), table.getMetaClient().getTableConfig(),
instantRange, Option.empty(), writeConfig.getProps());
- HoodieAppendHandle<IndexedRecord, ?, ?, ?> appendHandle =
CommonClientUtils.shouldWriteNativeLogs(
- writeConfig, table.getMetaClient().getTableConfig())
+ HoodieAppendHandle<IndexedRecord, ?, ?, ?> appendHandle =
CommonClientUtils.shouldWriteNativeLogs(writeConfig)
? new FileGroupReaderBasedNativeLogAppendHandle<>(writeConfig,
instantTime, table, operation, taskContextSupplier, readerContext)
Review Comment:
🤖 Confirmed the concern by tracing the code: `logCompact` builds the reader
context as a hardcoded `HoodieAvroReaderContext` (HoodieCompactor.java:175-176)
and isn't overridden by any engine subclass, so there's no Spark record-context
injection here like the delta-commit path gets. This looks like the same
degradation the author is fixing with the proposed `getRecordContextForWrite`
on `HoodieTable` (see comment on SparkNativeLogAppendHandle) — it would be
worth plumbing that same method into `logCompact` so the native Lance delete
writer gets a Lance record type instead of falling back to AVRO and hitting
`LANCE_UNSUPPORTED_ERROR_MSG`.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/compact/HoodieCompactor.java:
##########
@@ -167,15 +174,12 @@ public List<WriteStatus> compact(HoodieWriteConfig
writeConfig,
}
public List<WriteStatus> logCompact(HoodieWriteConfig writeConfig,
- CompactionOperation operation,
- String instantTime,
- Option<InstantRange> instantRange,
- HoodieTable table,
- TaskContextSupplier taskContextSupplier)
throws IOException {
- HoodieReaderContext<IndexedRecord> readerContext = new
HoodieAvroReaderContext(
- table.getStorageConf(), table.getMetaClient().getTableConfig(),
instantRange, Option.empty(), writeConfig.getProps());
- HoodieAppendHandle<IndexedRecord, ?, ?, ?> appendHandle =
CommonClientUtils.shouldWriteNativeLogs(
- writeConfig, table.getMetaClient().getTableConfig())
+ CompactionOperation operation,
+ String instantTime,
+ HoodieTable table,
+ TaskContextSupplier
taskContextSupplier,
+ HoodieReaderContext readerContext)
throws IOException {
Review Comment:
🤖 nit: `HoodieReaderContext` here is a raw type — the old signature used
`HoodieReaderContext<IndexedRecord>`. Could you use `HoodieReaderContext<?>`
(or the appropriate bound) to restore compiler type-checking on this parameter?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
