[api][core][spark] Support custom partition locations - #9540
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED
Implementation: FINDINGS
| requirePartitionManager().createPartitions(specs.asJava, ignoreIfExists) | ||
| } else { | ||
| requirePartitionManager() | ||
| .createPartitions(specs.asJava, ignoreIfExists, null, false, locations.asJava) |
There was a problem hiding this comment.
[P1] Validate the prospective registry atomically before registering locations.
This path validates each explicit location only against the table root and the default directory of that same spec, then immediately mutates the catalog. A batch can therefore assign the same or nested LOCATION values to different specs; MSCK can similarly add a default spec whose directory is already owned by another explicit spec. Registration succeeds, but scans, writes, and DROP later reject the full registry as overlapping, so normal SQL cannot even unregister the bad entry. Please make the catalog/REST mutation atomically validate the complete post-update ownership set for both ordinary create and /with-locations; a client-side listing alone would still race concurrent ADDs.
| // An absolute path without a scheme and file:/ name the same local filesystem. | ||
| scheme = scheme == null ? "file" : scheme.toLowerCase(Locale.ROOT); | ||
| String authority = uri.getAuthority(); | ||
| authority = authority == null ? "" : authority.toLowerCase(Locale.ROOT); |
There was a problem hiding this comment.
[P1] Canonicalize the filesystem identity before comparing ownership.
The ownership key uses the raw lower-cased scheme and authority, although different URI spellings can resolve to the same filesystem. For example, Hadoop can resolve hdfs://nn and hdfs://nn:8020 to the same NameNode, but this code treats them as separate roots. A table at hdfs://nn:8020/warehouse/t can therefore accept an explicit location such as hdfs://nn/warehouse/t, bypassing the table-root check and potentially reading the whole table as one partition or writing through a default sibling into explicitly owned data. Please derive the key from the resolved/canonical FileSystem URI, including default ports and configured aliases, and use that identity consistently for overlap checks and FileIO routing.
| String uriPath = uri.getPath(); | ||
| if (scheme == null | ||
| || scheme.isEmpty() | ||
| || uri.getUserInfo() != null |
There was a problem hiding this comment.
[P1] Do not reject URI forms used by supported filesystems.
These checks reject hdfs:///path because it has no authority, even though Paimon documents that form and HadoopFileIO can resolve it through the catalog Hadoop configuration. They also reject the standard ABFS form abfs://filesystem@account.dfs.core.windows.net/path, because java.net.URI parses filesystem as user info even though it is the Azure filesystem/container name rather than a credential. As a result, explicit partition locations are unusable for common HDFS deployments and standard ADLS Gen2 addresses. Please make validation scheme/FileIO-aware: qualify authorityless HDFS through CatalogContext, recognize the structural ABFS authority, and continue rejecting actual embedded credentials.
| if (RESTApi.containsCapability( | ||
| api.options().get(RESTCatalogInternalOptions.SERVER_CAPABILITIES), | ||
| RESTApi.FORMAT_TABLE_PARTITION_LOCATION_CAPABILITY) | ||
| && (count == null || count < 0)) { |
There was a problem hiding this comment.
[P1] Preserve the documented unknown-count fallback.
The OpenAPI description explicitly allows explicitPartitionLocationCount to be omitted, and Catalog#getExplicitPartitionLocationCount defines an empty result as a signal to load and validate the full registry. Requiring the count whenever the server advertises the location capability contradicts both contracts: a location-capable provider that cannot supply this optional optimization will make every getTable and table-detail conversion fail before any fallback can run. Please accept null as OptionalLong.empty() and use the existing full-registry path, or advertise count support through a separate capability that makes the field mandatory.
| override def run(sparkSession: SparkSession): Seq[Row] = { | ||
| val prefix = leadingPrefix(sparkSession) | ||
| val partitions = v2Table.partitionManager | ||
| val registeredPartitions = v2Table.partitionManager |
There was a problem hiding this comment.
[P2] Validate the full registry before scoped ANALYZE.
This query is already prefix-pruned, so checking location only on the returned partitions does not validate global path ownership or the authoritative explicit-location count. If selected default partition A has a directory that is also owned by an unselected explicit partition B, this guard passes and FormatTablePartitionStatsCollector reads that directory as A, probing explicitly owned data and replacing A statistics with B data. That state is reachable through the current ADD/MSCK registration gap or an inconsistent provider. Please run the same count-aware full-registry preflight used by scans and commits, then apply the ANALYZE prefix and reject selected explicit partitions.
JingsongLi
left a comment
There was a problem hiding this comment.
Incremental review of the latest revision.
| page_token = None | ||
| seen_page_tokens = set() | ||
| while True: | ||
| page = catalog.list_partitions_paged( |
There was a problem hiding this comment.
[P2] Avoid scanning the complete registry before every normal read. When no custom location exists, this loop must drain every page, and FormatTableScan.plan() invokes it unconditionally for every catalog-managed Format Table read (the first write does the same). A table with one million default-location partitions adds roughly 1,000 serial REST requests before planning can begin. Please expose an authoritative server-side has/count signal or negotiate a capability that lets PyPaimon reject unsupported custom-location tables without enumerating the entire registry.
| @JsonProperty(FIELD_LOCATION) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| @Nullable | ||
| private final String location; |
There was a problem hiding this comment.
I think location should be options, just like Table. Create partitions should also support options instead of location.
There was a problem hiding this comment.
More importantly, it is a highly specific variable—not a general-purpose one—so it should be tucked into the options object.
6b8ba71 to
f6a115f
Compare
Extend the Format Table partition manager with aligned locations. Support ADD PARTITION ... LOCATION while keeping DDL, ANALYZE, and MSCK behavior safe for catalog-managed partitions.
27c8633 to
c331145
Compare
Summary
Support Spark
ADD PARTITION ... LOCATIONfor internal, catalog-managed Format Tables. Custom locations are stored in the generic partitionoptionsmap underpath.Changes
POST .../partitionsrequest with position-alignedpartitionOptions. The endpoint and response stay unchanged, and unknown options are preserved.options["path"]in Core. Custom locations must be absolute, outside the table directory, and non-overlapping.locationto Paimonpath.DROP PARTITIONonly unregisters a custom location, andMSCK REPAIR TABLEleaves it unchanged.Testing
Notes
Upgrade the REST server and every reader before registering custom locations. PyPaimon does not support them yet and will be handled separately.
The commits are ordered REST API → Core → Spark.
Catalog implementations overriding the extended
createPartitionsmethod must accept the newpartitionOptionsargument.Follow-up to #8750 and #8751.