Skip to content

HIVE-29798: NumberFormatException while reading a table having UNION subdirs after ACID conversion - #6675

Open
abstractdog wants to merge 3 commits into
apache:masterfrom
abstractdog:HIVE-29798
Open

HIVE-29798: NumberFormatException while reading a table having UNION subdirs after ACID conversion#6675
abstractdog wants to merge 3 commits into
apache:masterfrom
abstractdog:HIVE-29798

Conversation

@abstractdog

@abstractdog abstractdog commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Fix two layout issues that prevent a non-ACID table loaded via INSERT ... UNION ALL from being read after ACID conversion.

1. Reader side — VectorizedOrcAcidRowBatchReader.java. When walking split parents looking for a base_* / delta_* / delete_delta_* ancestor, the "else" branch fed every non-base_* name to AcidUtils.ParsedDeltaLight.parse, which substrings past delta_ and calls Long.parseLong. On a HIVE_UNION_SUBDIR_15/ parent this throws NumberFormatException: For input string: "NION". Fixed by guarding with startsWith(DELTA_PREFIX) || startsWith(DELETE_DELTA_PREFIX) — mirroring the sibling non-vectorized OrcRawRecordMerger.TransactionMetaData#findWriteIDForSynthetcRowIDs.

2. Writer side — MoveTask.flattenUnionSubdirectories. When hive.tez.union.flatten.subdirectories=true, MoveTask renamed:

HIVE_UNION_SUBDIR_<N>/000000_0  →  <N>_000000_0     (old, three numeric parts)

That name has three numeric parts, so it matches neither ORIGINAL_PATTERN ([0-9]+_[0-9]+) nor ORIGINAL_PATTERN_COPY, and the ACID conversion validator rejects it with "Unexpected data file name format".

The fix folds the subdir index into the attempt-id portion of the writer name (arithmetic, not textual concatenation), keeping the flattened name in the plain writer-name namespace and out of the _copy_ namespace:

newAttempt = subdirIdx * 100_000 + originalAttempt

HIVE_UNION_SUBDIR_1/000000_0   →  000000_100000
HIVE_UNION_SUBDIR_23/000000_2  →  000000_2300002

Files touched:

  • ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java — add the isBase / isDelta guard around ParsedDeltaLight.parse.
  • ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java — flatten rename target changed from <N>_000000_0 to 000000_0_copy_<N> (uses Utilities.COPY_KEYWORD).

Why are the changes needed?

Reading a table after converting it to ACID currently crashes if the pre-conversion data was written by a UNION-ALL — regardless of the flatten setting:

  • flatten=OFF (default): HIVE_UNION_SUBDIR_<N>/000000_0 survives the conversion (metadata-only flip; TestTxnNoBuckets.testToAcidConversionMultiBucket pins this contract), but the reader crashes on the subdir name.
  • flatten=ON: conversion itself fails at TransactionalValidationListener.validateTableStructureForPath because <N>_000000_0 doesn't match either allowed original-file pattern.

Both ACID DDL paths — ALTER TABLE ... CONVERT TO ACID and the historical ALTER TABLE ... SET TBLPROPERTIES ('transactional'='true') (emitted by UpgradeTool) — are affected.

Does this PR introduce any user-facing change?

Yes — a bug fix. Users can now convert non-ACID tables loaded via INSERT ... UNION ALL to full ACID and read them, in both flatten modes. On-disk file-name change (flatten=on only): flattened union outputs are now written as 000000_0_copy_<N> instead of <N>_000000_0. No API or DDL syntax changes.

How was this patch tested?

New JUnit 5 test class ql/src/test/org/apache/hadoop/hive/ql/metadata/TestUnionAllToAcidConversion — 8 tests covering {unpartitioned, partitioned} × {CONVERT TO ACID, SET TBLPROPERTIES('transactional'='true')} × {flatten OFF, flatten ON}. Each test asserts the exact on-disk layout before and after conversion, then asserts SELECT COUNT(*) returns 3.

Asserted file layouts:

  • flatten=OFF: /<tbl>[/p=x]/HIVE_UNION_SUBDIR_{1,2,3}/000000_0 — preserved through conversion.
  • flatten=ON: /<tbl>[/p=x]/000000_0_copy_{1,2,3} — preserved through conversion (previously <N>_000000_0, which failed conversion).

Regression check:

TestTxnNoBuckets#testToAcidConversionMultiBucket           ✓  (pins the layout-preserved contract)
TestVectorizedOrcAcidRowBatchReader                        ✓  (17/17)
TestMoveTask / TestInsertCopySuffixOnFakeS3                ✓  (flatten call sites; updated for new name)
TestUnionAllToAcidConversion                               ✓  (8/8)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes UNION ALL directory/file layouts so non-ACID tables can be converted to ACID and read successfully.

Changes:

  • Skip non-ACID parent directories in the vectorized ACID reader.
  • Rename flattened union files to the ACID-compatible 000000_0_copy_N format.
  • Add conversion regression tests and update related expectations.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
ql/src/test/org/apache/hadoop/hive/ql/metadata/TestUnionAllToAcidConversion.java Updated as part of this pull request.
ql/src/test/org/apache/hadoop/hive/ql/metadata/TestInsertCopySuffixOnFakeS3.java Updated as part of this pull request.
ql/src/test/org/apache/hadoop/hive/ql/exec/TestMoveTask.java Updated as part of this pull request.
ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java Updated as part of this pull request.
ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java Updated as part of this pull request.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +159 to +160
fs.rename(path,
new Path(parentOfParent + "/" + path.getName() + Utilities.COPY_KEYWORD + parentNameSuffix));

@abstractdog abstractdog Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good catch, I had to redesign the logic used in movetask, addressed in: 2057a99

@sonarqubecloud

sonarqubecloud Bot commented Sep 4, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants