Copilot commented on code in PR #6866:
URL: https://github.com/apache/hive/pull/6866#discussion_r4250573157
##########
ql/src/java/org/apache/hadoop/hive/ql/io/HiveIgnoreKeyTextOutputFormat.java:
##########
@@ -67,16 +68,11 @@ public class HiveIgnoreKeyTextOutputFormat<K extends
WritableComparable, V exten
public RecordWriter getHiveRecordWriter(JobConf jc, Path outPath,
Class<? extends Writable> valueClass, boolean isCompressed,
Properties tableProperties, Progressable progress) throws IOException {
- int rowSeparator = 0;
String rowSeparatorString = tableProperties.getProperty(
serdeConstants.LINE_DELIM, "\n");
- try {
- rowSeparator = Byte.parseByte(rowSeparatorString);
- } catch (NumberFormatException e) {
- rowSeparator = rowSeparatorString.charAt(0);
- }
+ final byte[] rowSeparatorBytes =
+ rowSeparatorString.getBytes(StandardCharsets.UTF_8);
Review Comment:
This changes the historical semantics of `line.delim` when it is configured
as a numeric byte value (e.g., \"10\"): previously it would be parsed to a
single byte (LF), but now it will write the UTF-8 bytes for the characters '1'
and '0'. This is a behavior regression for existing tables that use numeric
delimiters. Consider preserving backward compatibility by detecting
purely-numeric values and converting them to a single byte (as before), while
still supporting multi-byte delimiters for non-numeric strings. Also consider
defining what should happen if `line.delim` is the empty string; currently it
results in writing no row separator bytes, which can concatenate records.
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/FetchOperator.java:
##########
@@ -141,6 +142,18 @@ public FetchOperator(FetchWork work, JobConf job,
Operator<?> operator,
this.job = job;
this.work = work;
this.operator = operator;
+ // When the table uses a multi-byte line.delim, propagate it to
+ // Hadoop's TextInputFormat so that LineRecordReader splits records
+ // on the full delimiter sequence instead of the default '\n'.
+ // FetchTask bypasses HiveInputFormat, so we must set it here.
+ if (work.getTblDesc() != null) {
+ String lineDelim = work.getTblDesc().getProperties()
+ .getProperty(serdeConstants.LINE_DELIM);
+ if (lineDelim != null && !lineDelim.isEmpty()
+ && !"\n".equals(lineDelim)) {
+ job.set("textinputformat.record.delimiter", lineDelim);
+ }
+ }
Review Comment:
The delimiter-propagation logic is duplicated here and in `HiveInputFormat`
(same property lookup + filtering + `job.set`). To reduce drift and ensure both
paths handle edge cases consistently (e.g., numeric delimiters, empty string
semantics), consider extracting a small shared helper (e.g., in `Utilities` or
a common IO utility) that resolves the table `line.delim` into the exact value
to set for `textinputformat.record.delimiter`.
##########
ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java:
##########
@@ -448,6 +449,16 @@ public RecordReader getRecordReader(InputSplit split,
JobConf job,
if ((part != null) && (part.getTableDesc() != null)) {
Utilities.copyTableJobPropertiesToConf(part.getTableDesc(), job);
nonNative = part.getTableDesc().isNonNative();
+ // When the table uses a multi-byte line.delim, propagate it to
+ // Hadoop's TextInputFormat so that LineRecordReader splits records
+ // on the full delimiter sequence instead of the default '\n'.
+ // TableDesc.getProperties() returns the raw (unescaped) value.
+ String lineDelim = part.getTableDesc().getProperties()
+ .getProperty(serdeConstants.LINE_DELIM);
+ if (lineDelim != null && !lineDelim.isEmpty()
+ && !"\n".equals(lineDelim)) {
+ job.set("textinputformat.record.delimiter", lineDelim);
+ }
Review Comment:
If `serdeConstants.LINE_DELIM` is set to a numeric byte string (e.g.,
\"10\"), this will configure Hadoop’s record delimiter to the literal
characters '1''0' instead of LF, which would break reading for such tables. If
numeric `line.delim` values are supported/expected, apply the same
backward-compatible decoding here as in the write path (convert numeric strings
to the corresponding single byte delimiter) before calling `job.set(...)`.
##########
ql/src/test/org/apache/hadoop/hive/ql/io/TestMultiCharLineDelimTextFormat.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * 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.hadoop.hive.ql.io;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.ql.exec.FileSinkOperator.RecordWriter;
+import org.apache.hadoop.hive.serde.serdeConstants;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.io.Text;
+import org.apache.hadoop.mapred.FileInputFormat;
+import org.apache.hadoop.mapred.InputSplit;
+import org.apache.hadoop.mapred.JobConf;
+import org.apache.hadoop.mapred.RecordReader;
+import org.apache.hadoop.mapred.Reporter;
+import org.apache.hadoop.mapred.TextInputFormat;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+/**
+ * Verifies that multi-byte {@code line.delim} round-trips correctly
+ * after the fix to {@link HiveIgnoreKeyTextOutputFormat} (write) and
+ * the {@code textinputformat.record.delimiter} propagation in
+ * {@link HiveInputFormat} (read), including field values that contain
+ * bare {@code \n} bytes.
+ */
+public class TestMultiCharLineDelimTextFormat {
+
+ @Rule
+ public TemporaryFolder tmpFolder = new TemporaryFolder();
+
+ private static final String LINE_DELIM = "\n";
+ private static final String FIELD_DELIM = "@_@";
+
+ @Test
+ public void testWriteThenReadMultiByteLineDelim() throws Exception {
+ // --- Write ---
+ Properties tblProps = new Properties();
+ tblProps.setProperty(serdeConstants.LINE_DELIM, LINE_DELIM);
+ tblProps.setProperty(serdeConstants.FIELD_DELIM, FIELD_DELIM);
+
+ JobConf job = new JobConf();
+ File outDir = tmpFolder.newFolder("write_test");
+ Path outPath = new Path(outDir.getAbsolutePath(), "000000_0");
+
+ HiveIgnoreKeyTextOutputFormat<?, ?> outputFormat =
+ new HiveIgnoreKeyTextOutputFormat<>();
+ RecordWriter writer = outputFormat.getHiveRecordWriter(
+ job, outPath, Text.class, false, tblProps, Reporter.NULL);
+
+ // Rows with \n inside field values. Field delimiter is already
+ // applied here; in real usage the SerDe does that.
+ String[] rows = {
+ "1001" + FIELD_DELIM + "gaoqiang" + FIELD_DELIM + "28",
+ "1002" + FIELD_DELIM + "an\nxin" + FIELD_DELIM + "27",
+ "1003" + FIELD_DELIM + "mengyu" + FIELD_DELIM + "25"
+ };
+ for (String row : rows) {
+ writer.write(new Text(row));
+ }
+ writer.close(false);
+
+ // Verify raw file content: each record ends with \002\n
+ byte[] fileBytes = Files.readAllBytes(outDir.toPath().resolve("000000_0"));
+ String fileContent = new String(fileBytes, StandardCharsets.UTF_8);
+ for (String row : rows) {
+ Assert.assertTrue("File should contain row + \\002\\n",
+ fileContent.contains(row + LINE_DELIM));
+ }
+
+ // --- Read ---
+ // Simulate what HiveInputFormat.getRecordReader does after the fix:
+ // read line.delim from table properties, then set
+ // textinputformat.record.delimiter on the JobConf.
+ JobConf readJob = new JobConf();
+ readJob.set("textinputformat.record.delimiter", LINE_DELIM);
+
+ FileInputFormat.setInputPaths(readJob, outPath);
+
+ TextInputFormat inputFormat = new TextInputFormat();
+ inputFormat.configure(readJob);
+
+ InputSplit[] splits = inputFormat.getSplits(readJob, 1);
+ Assert.assertEquals(1, splits.length);
+
+ RecordReader<LongWritable, Text> reader =
+ inputFormat.getRecordReader(splits[0], readJob, Reporter.NULL);
+
+ List<String> records = new ArrayList<>();
+ LongWritable key = reader.createKey();
+ Text value = reader.createValue();
+ while (reader.next(key, value)) {
+ records.add(value.toString());
+ }
+ reader.close();
+
+ Assert.assertEquals("Should read exactly 3 records", 3, records.size());
+ Assert.assertEquals(rows[0], records.get(0));
+ Assert.assertEquals(rows[1], records.get(1));
+ Assert.assertEquals(rows[2], records.get(2));
+ }
+
+ /**
+ * Verify that when line.delim is not set, the default \n splitting
+ * behaviour is preserved on both write and read paths.
+ */
+ @Test
+ public void testDefaultLineDelim() throws Exception {
Review Comment:
The new tests cover multi-byte delimiter strings and the default delimiter,
but they don't cover the backward-compatibility cases impacted by the
write/read changes (notably numeric `line.delim` values like \"10\" and the
empty-string case). Adding targeted assertions for those inputs would help
prevent regressions and validate the intended decoding semantics for
`line.delim`.
##########
ql/src/test/org/apache/hadoop/hive/ql/io/TestMultiCharLineDelimTextFormat.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * 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.hadoop.hive.ql.io;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.ql.exec.FileSinkOperator.RecordWriter;
+import org.apache.hadoop.hive.serde.serdeConstants;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.io.Text;
+import org.apache.hadoop.mapred.FileInputFormat;
+import org.apache.hadoop.mapred.InputSplit;
+import org.apache.hadoop.mapred.JobConf;
+import org.apache.hadoop.mapred.RecordReader;
+import org.apache.hadoop.mapred.Reporter;
+import org.apache.hadoop.mapred.TextInputFormat;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+/**
+ * Verifies that multi-byte {@code line.delim} round-trips correctly
+ * after the fix to {@link HiveIgnoreKeyTextOutputFormat} (write) and
+ * the {@code textinputformat.record.delimiter} propagation in
+ * {@link HiveInputFormat} (read), including field values that contain
+ * bare {@code \n} bytes.
+ */
+public class TestMultiCharLineDelimTextFormat {
+
+ @Rule
+ public TemporaryFolder tmpFolder = new TemporaryFolder();
+
+ private static final String LINE_DELIM = "\n";
Review Comment:
The new tests cover multi-byte delimiter strings and the default delimiter,
but they don't cover the backward-compatibility cases impacted by the
write/read changes (notably numeric `line.delim` values like \"10\" and the
empty-string case). Adding targeted assertions for those inputs would help
prevent regressions and validate the intended decoding semantics for
`line.delim`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
