sundapeng commented on PR #591:
URL: https://github.com/apache/paimon-rust/pull/591#issuecomment-5145292633
@JingsongLi Thanks for the review. All three are fixed, and the branch is
rebased onto current main (it was conflicting with #600 and #627). Details
below, including one place where I could not go all the way and would like your
call.
**1. DROP PARTITION semantics**
`DROP PARTITION` now takes several specifications per statement, and a
specification that fixes only some of the partition keys expands to every
registered partition it matches. The keys need not be a leading prefix, so
`DROP PARTITION (hh = '10')` on a `(dt, hh)` table works, matching
`PaimonFormatTable.dropFormatTablePartitions` where partial specs are matched
by arbitrary key subset. One catalog listing serves the whole statement,
however many specifications it carries.
Error semantics follow Java as well: a complete specification names one
partition, so a missing one raises unless `IF EXISTS`; a partial one describes
a set that is allowed to come out empty, so it is a no-op. A specification that
raises leaves the whole statement unapplied.
One thing I could not do. Hive and Spark write several specs as `DROP
PARTITION (a), (b)`, and sqlparser 0.62 cannot parse that: `parse_alter_table`
comma-separates ALTER *operations*, and a bare second `PARTITION` is read as
`RENAME PARTITION`. So multiple specs are written as repeated clauses:
```sql
ALTER TABLE t DROP PARTITION (dt = '20260722'), DROP PARTITION (dt =
'20260723');
```
The alternatives are a change in sqlparser, or a hand-rolled pre-parse of
the statement like the one `SHOW PARTITIONS` already uses here. I did not add
the pre-parse because it duplicates the ALTER TABLE grammar for one syntax
variant, but I will add it if you want the Hive spelling accepted.
I also did not add `list-by-names`. Java resolves complete specs through it
and asserts in tests that the complete-spec path never does a full traversal,
but paimon-rust has no `listPartitionsByNames` anywhere yet (no `Catalog`
method, no `RESTApi` method, no resource path, no request type). Adding the
whole stack here would grow a PR that is already large. The current code needs
the registered spec anyway to resolve the directory, so it costs one listing
per statement rather than one per specification. Happy to do it as a follow-up,
or here if you prefer.
**2. Partition pruning through the REST endpoint**
A catalog-managed scan now extracts the leading equality prefix from its
filter, builds the partition-name prefix pattern with the same contract as
`PartitionPathUtils.buildPartitionNamePrefixPattern` (escaped `key=value`
joined by `/`, `%` the only wildcard, complete prefix means the exact name,
shorter prefix gets `/%`, no pattern when a value is blank or escaping produced
a literal `%`), and sends it as `partitionNamePattern` with `maxResults=1000`.
The local per-partition match stays, so a catalog that ignores the pattern
still produces the same result set.
One Rust-specific detail worth flagging. A straight port of the Java
extractor would have pushed nothing in the case that matters most.
`PartitionFilter::from_predicate` collapses a filter that pins every partition
key into a `PartitionSet` and drops the predicate, so `WHERE dt = 'a' AND hh =
'10'` never reaches the scan as a `Predicate`. The extractor therefore also
handles `PartitionSet`, taking the longest common leading prefix of its rows. A
single-partition set gives the exact name, and `dt = 'a' AND hh IN ('10',
'11')` gives `dt=a/%`.
Predicate pushdown to `listPartitionsByFilter` is not included: #500 added
REST predicate JSON parsing but not serialization, so there is nothing to
encode with yet.
**3. Boolean partition values**
`parse_format_partition_value` now accepts `t/true/y/yes/1` and
`f/false/n/no/0` case-insensitively, mirroring `BinaryStringUtils.toBoolean`.
There was a unit test asserting that `yes` is rejected, which is exactly the
bug, so it is replaced with a table covering every spelling in both directions
plus the rejections.
While confirming this I found the same class of problem is wider than
booleans: `parse_format_partition_value` covers BOOLEAN, the integer types,
CHAR/VARCHAR, DATE and TIME, while `TypeUtils.castFromString` also covers
DECIMAL, FLOAT, DOUBLE, TIMESTAMP, TIMESTAMP_LTZ and BINARY. A catalog-managed
table partitioned by any of those cannot be scanned. That is not a regression
from this PR, and closing it properly means aligning both directions of the
conversion, which are currently split across two modules that have already
drifted. I would rather do it in its own PR than widen this one. Tell me if you
want it here instead.
**Also in this push**
`docs/src/sql.md` documents all four statements, and the PR description now
follows the template.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
