JingsongLi commented on code in PR #9540:
URL: https://github.com/apache/paimon/pull/9540#discussion_r3921081037
##########
paimon-api/src/main/java/org/apache/paimon/rest/RESTApi.java:
##########
@@ -996,20 +996,24 @@ public void markDonePartitions(Identifier identifier,
List<Map<String, String>>
* PartitionStatistics#spec()} rather than by position, or null to
report none
* @param replaceStatistics whether the report replaces the stored values
rather than adding to
* them; ignored when {@code statistics} is null, and not sent at all
in that case
+ * @param partitionLocations custom locations aligned with {@code
partitions} by position, where
+ * null entries use derived defaults; null omits the location extension
* @return the partitions the server created and the ones it already held
*/
public CreatePartitionsResponse createPartitions(
Identifier identifier,
List<Map<String, String>> partitions,
boolean ignoreIfExists,
@Nullable List<PartitionStatistics> statistics,
- boolean replaceStatistics) {
+ boolean replaceStatistics,
+ @Nullable List<String> partitionLocations) {
CreatePartitionsRequest request =
new CreatePartitionsRequest(
partitions,
ignoreIfExists,
statistics,
- statistics == null ? null : replaceStatistics);
+ statistics == null ? null : replaceStatistics,
+ partitionLocations);
Review Comment:
**[P1] Do not send locations through a route that legacy servers accept
silently.** A pre-feature server deserializes this request with
`@JsonIgnoreProperties(ignoreUnknown = true)`, so it discards
`partitionLocations`, registers the spec at the derived default location, and
returns the unchanged successful response. The new client therefore treats `ADD
PARTITION ... LOCATION` as successful even though later scans will not read the
requested external directory. The current mock `501` test only models an
upgraded server that recognizes the field. Please use a location-specific
endpoint or a mandatory capability/response acknowledgement that an old server
cannot accidentally satisfy, and add a mixed-version test against the base DTO
behavior.
##########
paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java:
##########
@@ -203,6 +207,8 @@ public void commit(List<CommitMessage> commitMessages) {
}
}
+ List<Partition> validatedPartitions =
rejectWritesToCustomLocationPartitions(messages);
Review Comment:
**[P1] Make the default-location ownership check atomic with registration.**
This is only a preflight: after it returns, a concurrent `ADD PARTITION ...
LOCATION` can register one of the affected specs before the writer publishes
and calls `createPartitions` with no location assertion. Because
`ignoreIfExists` is true, registration succeeds against the now-custom
partition and its statistics may be attached to that partition even though the
new files were published under the default path, making the committed rows
invisible. Please pass a position-aligned all-null `partitionLocations` list
for these preflighted default-owned specs so the server rejects an intervening
custom owner (and abort removes the published files), and cover the
interleaving with a latch-based test.
##########
paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java:
##########
@@ -1072,35 +1072,23 @@ default void createPartitions(Identifier identifier,
List<Map<String, String>> p
throws TableNotExistException {}
/**
- * Create partitions of the specify table, with explicit existence
semantics and optionally
- * reporting statistics for them in the same call.
- *
- * <p>The statistics are matched to {@code partitions} by {@link
PartitionStatistics#spec()}, so
- * they may cover only some of them, and {@code replaceStatistics} says
whether they replace
- * what the catalog already holds or add to it. What decides whether they
survive is whether a
- * catalog overrides this method: one that does not registers the
partitions exactly as {@link
- * #createPartitions(Identifier, List)} does and drops the report, however
much of it the
- * catalog could have stored, and for a catalog that keeps no partitions
at all that means it
- * does nothing.
- *
- * @param identifier path of the table to create partitions
- * @param partitions partitions to be created
- * @param ignoreIfExists if false, fail when any partition already exists
and apply none of the
- * batch; if true, behave like {@link #createPartitions(Identifier,
List)}
- * @param statistics statistics to report, or null to report none
- * @param replaceStatistics whether the report replaces the stored values
rather than adding to
- * them; ignored when {@code statistics} is null
- * @throws TableNotExistException if the table does not exist
- * @throws UnsupportedOperationException if {@code ignoreIfExists} is
false and the catalog does
- * not implement strict creation, which is what the default here does
+ * Create partitions atomically unless existing entries are ignored, with
optional statistics
+ * and position-aligned locations whose null entries use defaults.
*/
default void createPartitions(
Identifier identifier,
List<Map<String, String>> partitions,
boolean ignoreIfExists,
@Nullable List<PartitionStatistics> statistics,
- boolean replaceStatistics)
+ boolean replaceStatistics,
+ @Nullable List<String> partitionLocations)
Review Comment:
**[P1] Preserve the existing public overload.** This replaces the
five-argument `createPartitions(identifier, partitions, ignoreIfExists,
statistics, replaceStatistics)` method that exists on the base branch of the
`@Public` `Catalog` interface. Existing source integrations will no longer
compile and already-compiled callers can fail with `NoSuchMethodError`; an
existing catalog implementation that overrides only the old method is also
bypassed by new six-argument callers, potentially dropping statistics through
the default fallback. Please keep the old overload and make the new overload
delegate to it when `partitionLocations` is null (and retain analogous bridges
in `RESTApi` and `FormatTablePartitionManager`).
##########
paimon-python/pypaimon/table/format/format_table.py:
##########
@@ -89,6 +92,44 @@ def format(self) -> Format:
def options(self) -> Dict[str, str]:
return self._options
+ def _has_custom_partition_locations(self) -> bool:
+ catalog_environment = getattr(self, "_catalog_environment", None)
+ if catalog_environment is None:
+ return False
+ catalog = catalog_environment.catalog_loader.load()
+ page_token = None
+ seen_page_tokens = set()
+ while True:
+ page = catalog.list_partitions_paged(
Review 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.
--
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]
