voonhous commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3832454704
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>For {@code SELECT COUNT(*)} on Hive before 3.0.0 the read-column ids
arrive empty and Hive combines
+ * them into e.g. {@code ",2,0,3"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
+ * {@code HoodieColumnProjectionUtils#getReadColumnIDs} and
+ * {@code HoodieRealtimeRecordReaderUtils#orderFields} all do this. Cleaning
the conf once here covers all
+ * of them, including the bootstrap path that never reaches {@code
orderFields}.
Review Comment:
Not true for COW, and the same claim is in the commit message.
None of the three call sites is in `HoodieParquetInputFormat`, which is
where the other two consumers are reached: `:154` ->
`createBootstrappingRecordReader` -> `:193 getReadColumnIDs` -> `parseInt`, and
`:159` -> `SchemaEvolutionContext:259 parseInt`. `shouldUseFilegroupReader`
(`HoodieInputFormatUtils:568-572`) forces both onto the legacy path, and
`TestHiveTableSchemaEvolution:233` shows the cow arm using `new
HoodieParquetInputFormat()` directly.
Add `cleanProjectionColumnIds(job)` after `HoodieParquetInputFormat:121`
(idempotent, so `:129` becomes a no-op), or drop the bootstrap clause here, in
the commit message, and in the PR body table.
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>For {@code SELECT COUNT(*)} on Hive before 3.0.0 the read-column ids
arrive empty and Hive combines
+ * them into e.g. {@code ",2,0,3"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
+ * {@code HoodieColumnProjectionUtils#getReadColumnIDs} and
+ * {@code HoodieRealtimeRecordReaderUtils#orderFields} all do this. Cleaning
the conf once here covers all
Review Comment:
The third call site does not count either.
`HoodieFileGroupReaderBasedRecordReader:114` cleans `jobConfCopy`, but
`HiveHoodieReaderContext.setSchemas` (`:113-115`, called at `:175`) overwrites
both `READ_COLUMN_NAMES` and `READ_COLUMN_IDS` from the requested schema before
the reader is built, and nothing in between reads the ids
(`createRequestedSchema:317-323` uses names only).
Drop it as a cited consumer here and in the PR body.
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>For {@code SELECT COUNT(*)} on Hive before 3.0.0 the read-column ids
arrive empty and Hive combines
+ * them into e.g. {@code ",2,0,3"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
Review Comment:
`setColumnTypeList` is cited here as protected, but it still has the bug you
fixed one method below: `job.get(READ_COLUMN_IDS_CONF_STR)` with no default at
`SchemaEvolutionContext:259`, `:263`, `:392`. Same missing-default NPE that
`dd31c9b5d8dc` (#10313) fixed in `addProjectionField`.
`setColumnNameList` (`:388-404`) is missing from the list. It runs first
(`:250`) with the same hazards plus an IOOBE on `fullColNamelist.get(id)`.
Add the `, ""` default at those three lines or file a follow-up, and add
`setColumnNameList` to the list above.
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>For {@code SELECT COUNT(*)} on Hive before 3.0.0 the read-column ids
arrive empty and Hive combines
+ * them into e.g. {@code ",2,0,3"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
+ * {@code HoodieColumnProjectionUtils#getReadColumnIDs} and
+ * {@code HoodieRealtimeRecordReaderUtils#orderFields} all do this. Cleaning
the conf once here covers all
+ * of them, including the bootstrap path that never reaches {@code
orderFields}.
+ *
+ * <p>This is a workaround: the underlying bug is in Hive, fixed after
3.0.0, but earlier versions still
+ * hit it. Stripping a single leading comma is not enough. Hive prepends ids
while appending names, so repeated
+ * empty appends give {@code ",,2,0"}, and an id prepended after an empty
one gives {@code "3,,2,0"} where
+ * the blank is interior and no amount of leading-comma stripping reaches it.
*/
public static void cleanProjectionColumnIds(Configuration conf) {
Review Comment:
Judgement call, not reproduced. This is an unsynchronized read-modify-write
on a JobConf the caller locks, and the change widens it: the write used to fire
only on a leading comma, now on any blank or padded token.
`ee0fd06de73e` put `synchronized (conf)` inside this method; `3a05edab01f7`
removed it and moved the call into the caller's latch; `f41539a9cb5f` (#3630)
moved the call back out without restoring the lock.
Wrap the body in `synchronized (conf)`. Do not move the call back into the
latch, that reverts `f41539a9cb5f`.
--
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]
