Fix flaky format detection in Hive coercion tests - #30994
Conversation
| List<Object> hexRepresentedValue = ImmutableList.of("58EFBFBDEFBFBDEFBFBDEFBFBD", "58EFBFBDEFBFBDEFBFBDEFBFBD58"); | ||
|
|
||
| if (tableName.toLowerCase(ENGLISH).contains("orc")) { | ||
| if (tableName.toLowerCase(ENGLISH).startsWith("orc")) { |
There was a problem hiding this comment.
Why do we use tableName instead of fileFormat?
There was a problem hiding this comment.
I'm not sure. I can convert to use it, but I tried to keep the changes minimal.
There was a problem hiding this comment.
Almost everything was able to use fileFormat, but for RCFILE there was a storage format (i.e. - RCTEXT/RCBINARY) nuance. My best guess is, the tableName based check was likely for consistency since the tableName could carry either one.
Given that, I took some liberty and cleaned it up a little bit to better disambiguate the variables. The recommendTableName was really just fileFormat/storageFormat that was encoded into the tableName. Instead it now carries fileFormat/storageFormat forward to the required methods instead of relying on it being encoded into the tableName.
The Hive coercion tests rediscovered the storage format by searching the table name for a format token, and the name ends in a random ten-character suffix over [0-9a-z]. A suffix containing "orc" made a non-ORC table look like ORC, silently switching expected values. Pass the format down instead, so the table name is only used to address the table. RCTEXT and RCBINARY share the RCFILE file format but not their expectations, so an optional storage format carries that distinction.
8bddbc2 to
4fbb6e3
Compare
2 participants
Description
The Hive coercion product tests rediscovered the storage format by substring-matching
the table name, which ends in a random ten-character suffix over
[0-9a-z]:A suffix containing
orcmade a non-ORC table look like ORC and silently switched theexpected values. That is about 1 in 5,800 per table name, and
SuiteHiveStorageFormatscreates roughly 150 tables per run — so it fails a few percent of runs and is always
green on re-run.
Every caller already knows the format, so pass it down rather than rediscovering it; the
table name is now used only to address the table.
fileFormatalone is not enough,because it is
RCFILEfor both RCTEXT and RCBINARY, which share aSTORED ASclause butnot their expectations — an optional storage format carries that distinction.
Additional context and related issues
Verified by exercising the expectation logic directly. Every format-derived expectation
for both engines and all six formats, plus both column-removal lists (1,696 lines), is
identical to master. Against an adversarial table name, master reproduces both reported
failures and this branch clears them:
double_to_string[12345.12345, null][12345.12345, NaN]assertNestedSubFields[null, null][2, 2]Both issues predate the split into the current two test classes, so their line numbers
refer to
BaseTestHiveCoercion, but the predicate carried over unchanged.Release notes
(x) This is not user-visible or is docs only, and no release notes are required.