fix external parquet hive partition issue - #24468
Merged
Merged
Conversation
(cherry picked from commit 5dc2390)
(cherry picked from commit 3cf4306)
(cherry picked from commit dc7dfd1)
(cherry picked from commit 3f22d0a)
(cherry picked from commit 5bf4075)
(cherry picked from commit df6be17)
(cherry picked from commit e8b1639)
(cherry picked from commit b094772)
(cherry picked from commit c02c101)
(cherry picked from commit ee74454)
(cherry picked from commit 3416b81)
(cherry picked from commit 6a10388)
(cherry picked from commit 306919f)
(cherry picked from commit 8842931)
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
heni02
approved these changes
May 19, 2026
aunjgr
approved these changes
May 19, 2026
aunjgr
left a comment
aunjgr
left a comment
Contributor
There was a problem hiding this comment.
Review
Previously flagged issues from #24450 — all fixed ✅
- uint64→int64 boundary: now uses
val > uint64(math.MaxInt64)— correctly rejects 0x8000000000000000. - S3 prefetch OOM:
maxParquetS3PrefetchSize = 128 MBcap in place; files larger than 128 MB fall back to streaming reads. - Page state cleanup on error: page resources are only acquired after successful mapper creation, so pre-allocated empty slices on error paths are harmless.
ALTER TABLE ADD INDEX bypass — not a bug ✅
All three index variants in buildAlterTableInplace (*tree.UniqueIndex at line 4046, *tree.FullTextIndex at line 4101, *tree.Index at line 4158) call checkCreateIndexTableType, so the external-table rejection is properly enforced on ALTER TABLE paths too.
BETWEEN lo > hi → MatchFalse — correct ✅
x BETWEEN a AND b is equivalent to a <= x AND x <= b. When a > b no value can satisfy this, so returning MatchFalse and pruning the partition is the correct SQL-standard behavior.
Minor: no boundary value tests for the two fixed conversions
The new parquet_coverage_test.go doesn't cover:
- UINT64 =
math.MaxInt64(9223372036854775807) → BIGINT: should succeed - UINT64 =
math.MaxInt64 + 1(9223372036854775808) → BIGINT: should error - S3 file exactly 128 MB → should prefetch; 128 MB + 1 B → should stream
These are non-blocking but would lock in the fix against regressions. Consider adding them in a follow-up.
LGTM overall.
Contributor
Merge Queue Status
This pull request spent 2 hours 7 minutes 46 seconds in the queue, including 1 hour 7 seconds running CI. Required conditions to merge
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24374 #24364 #24370 #24372 #24385 #24375 #24360
What this PR does / why we need it:
fix external parquet hive partition issue