hudi-agent commented on code in PR #19283:
URL: https://github.com/apache/hudi/pull/19283#discussion_r3578371148
##########
hudi-client/hudi-spark-client/src/main/scala/org/apache/hudi/SparkFileFormatInternalRowReaderContext.scala:
##########
@@ -144,10 +144,9 @@ class
SparkFileFormatInternalRowReaderContext(baseFileReader: SparkColumnarFileR
// Parquet stores VECTOR as FIXED_LEN_BYTE_ARRAY, so the reader needs
BinaryType
// and we decode back to ArrayType below. Lance returns ArrayType
natively, so skip
// the rewrite only for Lance base files; log files always go through the
rewrite path.
Review Comment:
🤖 nit: this comment now contradicts the code below it — `isLanceFile` (line
148) is `!isInlineLog && tableConfig.getBaseFileFormat == LANCE`, so native
Lance log files also skip the vector rewrite, but the comment still says "log
files always go through the rewrite path". Could you update it to something
like "the rewrite only for Lance files (base and native log); inline log files
always go through the rewrite path" so future readers aren't misled?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 Now that Lance passes shouldWriteNativeLogs, this builds a
FileGroupReaderBasedNativeLogAppendHandle with a hardcoded
HoodieAvroReaderContext, but unlike the delta-commit path it doesn't get the
Spark record-context override that SparkNativeLogAppendHandle adds. On Spark
executors the handle's record context degrades to Avro (the same degradation
your comment in SparkNativeLogAppendHandle describes), so
HoodieNativeLogFormatWriter.ensureDeleteFileWriter creates the .deletes.lance
writer with record type AVRO -- and HoodieAvroFileWriterFactory has no Lance
writer, so it throws LANCE_UNSUPPORTED_ERROR_MSG. Have you been able to run
Lance MOR log compaction end-to-end? @nsivabalan could you sanity-check this
path?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-spark-client/src/main/scala/org/apache/hudi/SparkFileFormatInternalRowReaderContext.scala:
##########
@@ -144,10 +144,9 @@ class
SparkFileFormatInternalRowReaderContext(baseFileReader: SparkColumnarFileR
// Parquet stores VECTOR as FIXED_LEN_BYTE_ARRAY, so the reader needs
BinaryType
// and we decode back to ArrayType below. Lance returns ArrayType
natively, so skip
// the rewrite only for Lance base files; log files always go through the
rewrite path.
- val isLogFile = FSUtils.isLogFile(filePath)
- val isLanceBaseFile = !isLogFile && FSUtils.isBaseFile(filePath) &&
- tableConfig.getBaseFileFormat == HoodieFileFormat.LANCE
- val vectorColumnInfo: Map[Int, HoodieSchema.Vector] = if (isLanceBaseFile)
{
+ val isInlineLog = FSUtils.isInlineLogFile(filePath.getName)
+ val isLanceFile = !isInlineLog && tableConfig.getBaseFileFormat ==
HoodieFileFormat.LANCE
Review Comment:
🤖 I traced this and the mismatch looks real. A native Lance log is named
`<fileId>_<writeToken>_<instant>_<version>.log.lance` (see
`FileNameParser.NATIVE_LOG_FILE_PATTERN`), so `filePath.getFileExtension()`
returns `.lance` and `fromFileExtension` at line 174 does dispatch it to the
Lance reader by extension. But at line 148 `isLanceFile` keys off
`tableConfig.getBaseFileFormat`, so in a non-Lance-default multi-format table
it's `false` → `vectorColumnInfo` gets populated and the schema is
binary-rewritten. Worth noting the `wrapWithVectorConversion` post-processing
at line 207 only runs in the non-log branch, so nothing converts it back on the
log path either. Keying `isLanceFile` off `filePath.getFileExtension`
(consistent with line 174) would keep the two decisions aligned.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/io/SparkAppendHandleFactory.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.hudi.io;
+
+import org.apache.hudi.common.engine.TaskContextSupplier;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.config.HoodieWriteConfig;
+import org.apache.hudi.table.HoodieTable;
+import org.apache.hudi.util.CommonClientUtils;
+
+import java.util.Iterator;
+
+/**
+ * Spark-specific append-handle factory for native logs.
+ *
+ * <p>It selects {@link SparkNativeLogAppendHandle} only after native logs are
enabled. The handle then uses the
+ * write config's record type to decide whether delete records must be
represented as Spark rows or Avro records.</p>
Review Comment:
🤖 nit: "only after native logs are enabled" reads like a temporal sequence
("after some enabling event") rather than a condition — could you change it to
"only when native logs are enabled"?
<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]
