ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

Solid feature — the test coverage (hive_partition_test.go, hive_partition_coverage_test.go, BVT) is unusually thorough and the partition-pruning hookup into the optimizer is well-isolated. A few things worth addressing before merge; none are blockers.

Correctness / UX

1. __HIVE_DEFAULT_PARTITION__ unconditionally mapped to NULL (hive_partition_fill.go, the if strVal == HiveDefaultPartition block). The check fires for every column type, including VARCHAR / TEXT. A user whose string partition legitimately contains that literal token (e.g., data imported from a system that didn't treat it as a sentinel) will see those rows silently NULL-ified. Hive/Spark do the same thing globally, so this is defensible — but worth calling out in user docs and/or restricting the sentinel semantics to non-string types where the literal string could never otherwise be the partition value.

2. Zero-padded numeric partitions don't equate to unpadded predicates (matchPartitionValue in hive_partition.go). Directory month=01 compared against WHERE month=1 — if the partition column is typed INT, both coerce to 1 and prune correctly. But if typed VARCHAR, "01" != "1" and pruning misses. Test fixture zero_pad/ uses INT so this is fine in practice; worth a test case with VARCHAR to lock the behavior in both directions.

3. NOT NULL DEFAULT + __HIVE_DEFAULT_PARTITION__ error message shows the relative path but not the column default. Minor UX: the user sees "NOT NULL but directory has HIVE_DEFAULT_PARTITION" and has to guess whether to add NULL default or rename the directory. A short suggestion in the error message would help.

Hardening

4. URL-decoding path is dead code. hive_partition.go:~265 hard-rejects any directory name containing % before listing descends into it. ParseHivePartitionSegment calls url.PathUnescape on the value portion, but the guard above ensures no % ever reaches that code. Either remove the guard and let the URL-decoding actually work (the url_encoded fixture is labeled "P0 known limitation" — escalating it to P1 would be a nice follow-up), or remove the PathUnescape and comment that URL-encoding is unsupported. Having both is confusing.

5. Partition pruning only handles = and IN (tryExtractPredicate, hive_partition.go). <, <=, >, >=, BETWEEN, NOT IN all fall through to row-level filtering. Not a correctness issue (the predicate is also applied row-side), but the perf benefit of partitioning vanishes for range queries — which is exactly what partitions are usually for on date/year columns. Worth tracking as a follow-up.

6. collectBareColNames only recurses into Expr_F (hive_partition.go). Expressions wrapped in CAST(...) or inside CASE / subqueries don't surface their referenced columns, so pruning is silently skipped. A predicate like CAST(year AS INT) = 2024 won't prune. The row-level filter still catches it, so it's conservative — but the pruning surface is narrower than the design doc implies. Consider walking through CAST and CASE-WHEN nodes.

7. Serial directory listing, 10k list-call cap (DiscoverHivePartitions). On S3 at ~100ms/list a table with 5000 partitions takes ~8 min to plan. Not a bug — safe by design — but worth a bounded concurrent list (e.g., errgroup.Group with SetLimit(8)) as a follow-up. The TODO at hive_partition.go:~91 about re-invoking GetForETLWithType per recursion compounds this; caching the FS client across the recursion would help.

8. validateLiteralVecBinary hand-parses vector.Vec.MarshalBinary layout (hive_partition.go:~731). The format isn't versioned. If the vector binary layout changes, this silently returns false and partition pruning stops working with no observable error. Replace with vec.UnmarshalBinary + Length check.

9. parseHiveOptionKV defensive skip-guards against legacy JSON — if a legacy JSON has hive_partitioning=false but hive_partition_columns=year, the table ends up with columns but pruning disabled. No consumer guards against this inconsistent state (refreshPartitionValues checks HivePartitioning only). Consider rejecting the combination in validateAndSetHivePartitionOptions to fail fast.

Verified correct

• Parser change (update.go, 14 lines): ExternParam gets HivePartitioning, HivePartitionCols, HivePartitionColTypes (with a local HivePartColType to avoid importing pkg/pb/plan). Plumbed end-to-end through build/compile/exec. Reasonable boundary.
• Concurrency (types.go, 11 lines): new per-ExternalParam state is read/written by a single reader goroutine because param.Parallel = false is forced for hive in compile.go. Safe as long as that enforcement holds — worth a comment on each new field noting the Parallel=false dependency.
• Hidden files: IsHiddenFile correctly skips _SUCCESS and .crc; non-parquet regular files are silently filtered. Consistent with Hive convention.
• Path boundary check: ExtractPartitionValues normalizes via path.Clean then enforces HasPrefix(filePath, basePath+"/"). path.Clean collapses .. before the check, so basePath/../etc/passwd → /etc/passwd fails the prefix test. Consistent with non-hive external-table path handling.

done