HIVE-29388: Operations on parquet tables with qouted identifier columns run into exceptions#6506
HIVE-29388: Operations on parquet tables with qouted identifier columns run into exceptions#6506tanishq-chugh wants to merge 6 commits into
Conversation
asf-ci-hive
added
tests pending
tests unstable
and removed
tests pending
tests unstable
labels
|
| Map<String, String> metadata) { | ||
| String columnNames = configuration.get(IOConstants.COLUMNS); | ||
| String columnTypes = configuration.get(IOConstants.COLUMNS_TYPES); | ||
| if (columnNames != null && !columnNames.isEmpty() && columnTypes != null && !columnTypes.isEmpty()) { |
There was a problem hiding this comment.
Can we simplify these conditions using StringUtils.isNotEmpty?
There was a problem hiding this comment.
Hi @difin , Thanks for checking this!
We were already importing org.apache.hadoop.util.StringUtils in DataWritableReadSupport, so instead of using the fully-qualified StringUtils naming here, i have changed it to use StringUtils.hasLength() from same hadoop util in commit: 62dcff2
Let me know if this is fine?
| public static MessageType getSchema(final Configuration configuration) { | ||
| String columnNames = configuration.get(IOConstants.COLUMNS); | ||
| String columnTypes = configuration.get(IOConstants.COLUMNS_TYPES); | ||
| if (columnNames != null && !columnNames.isEmpty() && columnTypes != null && !columnTypes.isEmpty()) { |
There was a problem hiding this comment.
same, can we use StringUtils.isNotEmpty here?
| return split; | ||
| } | ||
|
|
||
| private ParquetInputSplit buildParquetInputSplit(final Path finalPath, final MessageType requestedSchema) |
There was a problem hiding this comment.
Why was it required to replace the old split constructor with a new one for quoted-identifier schema handling?
There was a problem hiding this comment.
The old constructor expected the schema in string, which was internally parsed in ParquetInputSplit.end() using:
MessageTypeParser.parseMessageType(requestedSchema)
running into the original parsing issue for Read, where parquet interpreted the identifier inside the parentheses to be datatype.
Have changed it to compute the size & offset in buildParquetInputSplit() using our MessageType requestedSchema object avoiding string schema parsing and call the new constructor which is the same functionality of the old constructor (It also computed the size/offset and then calls this new constructor internally).
There was a problem hiding this comment.
Can we add additional test coverage?
- Nested/complex column names with quotes inside struct/list/map types.
- A test to select only one column from a table that also has the quoted column, so Hive reads a pruned schema, but still needs the full table schema internally - for example, with your
parquet_bps_sinktable:select id from parquet_bps_sink order by id;
There was a problem hiding this comment.
Right, it would be better to have this coverage. Updated the Qtest with the same in commit: c4fb443
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
tests unstable
labels
|
3 participants
What changes were proposed in this pull request?
Fix select & insert queries on parquet tables with qouted identifier columns running into errors
Why are the changes needed?
To fix errors in select & insert queries on parquet tables with qouted identifier columns
Does this PR introduce any user-facing change?
Yes, Queries initially running into errors will now run successfully
How was this patch tested?
Manual Testing + have added a QTest with failing coverage