[GitHub] [incubator-paimon] JingsongLi commented on a diff in pull request #1008: [core] add interface listPartitions and dropPartitions to paimon Catalog

["JingsongLi (via GitHub)" <gi...@apache.org>]

JingsongLi commented on code in PR #1008:
URL: https://github.com/apache/incubator-paimon/pull/1008#discussion_r1175942606


##########
paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java:
##########
@@ -182,6 +183,30 @@ void renameTable(Identifier fromTable, Identifier toTable, boolean ignoreIfNotEx
     void alterTable(Identifier identifier, List<SchemaChange> changes, boolean ignoreIfNotExists)
             throws TableNotExistException;
 
+    /**
+     * List partitions by the given identifier
+     *
+     * <p>NOTE: cause paimon use dynamic partition all the time, Exists Partitions is not a table
+     * properties cached in memory, the method will cause io read every time it called.
+     *
+     * @param identifier path of the table
+     * @return a list of partitions of the specified table in identifier
+     * @throws TableNotExistException if the table does not exist
+     */
+    List<Partition> listPartitions(Identifier identifier) throws TableNotExistException;

Review Comment:
   for compatibility, new methods in public api should have default implementation, like throw unsupported exception.



##########
paimon-core/src/main/java/org/apache/paimon/operation/AbstractFileStoreScan.java:
##########
@@ -272,6 +277,116 @@ public List<ManifestEntry> files() {
         };
     }
 
+    // this method read partitions in Manifest File partition related bytes directly, reduce the
+    // usage of redundant deserialization
+    public List<PartitionEntry> partitions() {

Review Comment:
   Maybe `planPartitions`? I want to emphasize that there is plan action here.
   
   BTW, don't forget add `@Override`



##########
paimon-core/src/main/java/org/apache/paimon/operation/AbstractFileStoreScan.java:
##########
@@ -272,6 +277,116 @@ public List<ManifestEntry> files() {
         };
     }
 
+    // this method read partitions in Manifest File partition related bytes directly, reduce the
+    // usage of redundant deserialization
+    public List<PartitionEntry> partitions() {
+        List<ManifestFileMeta> manifests = specifiedManifests;
+        Long snapshotId = specifiedSnapshotId;
+        if (manifests == null) {
+            if (snapshotId == null) {
+                snapshotId = snapshotManager.latestSnapshotId();
+            }
+            if (snapshotId == null) {
+                manifests = Collections.emptyList();
+            } else {
+                Snapshot snapshot = snapshotManager.snapshot(snapshotId);
+                manifests = readManifests(snapshot);
+            }
+        }
+
+        Iterable<PartitionEntry> entries =
+                parallelismBatchIterable(
+                        files ->
+                                files.parallelStream()
+                                        .flatMap(m -> readPartitionEntries(m).stream())
+                                        .collect(Collectors.toList()),
+                        manifests,
+                        400);
+
+        return PartitionEntry.mergePartitionEntries(entries);
+    }
+
+    // reduce memory usage by batch iterable process, the result in memory while be 2 * queueSize
+    public static <T, U> Iterable<T> parallelismBatchIterable(

Review Comment:
   Can this be a util class? It seems a generic util.
   
   And we can have unit tests for this.



##########
paimon-core/src/main/java/org/apache/paimon/partition/Partition.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.partition;
+
+import java.util.LinkedHashMap;
+import java.util.Objects;
+
+/** Partition with the specified values. */

Review Comment:
   This should be a public class.
   ```
    *
    * @since 0.4.0
    */
   @Public
   ```



##########
paimon-core/src/main/java/org/apache/paimon/partition/PartitionUtils.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.partition;
+
+import org.apache.paimon.types.RowType;
+
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/** Utils for partition. */
+public class PartitionUtils {
+
+    public static List<Partition> convertToPartitionListFromEntry(

Review Comment:
   Just inline this method to the invoker.



##########
paimon-core/src/main/java/org/apache/paimon/partition/PartitionEntry.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.partition;
+
+import org.apache.paimon.data.BinaryRow;
+import org.apache.paimon.manifest.FileKind;
+import org.apache.paimon.types.DataField;
+import org.apache.paimon.types.RowType;
+import org.apache.paimon.types.TinyIntType;
+import org.apache.paimon.utils.Preconditions;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import static org.apache.paimon.utils.SerializationUtils.newBytesType;
+import static org.apache.paimon.utils.SerializationUtils.newStringType;
+
+/** Partition Entry pojo read from manifest file. */
+public class PartitionEntry {

Review Comment:
   Maybe we can name this class `SimpleManifestEntry`:
   ```
   SimpleManifestEntry {
      FileKind kind;
      BinaryRow partition;
      int bucket;
      String fileName;
   }
   ```
   
   It must contain `bucket` and `fileName` too.



##########
paimon-core/src/main/java/org/apache/paimon/table/source/TableScan.java:
##########
@@ -34,6 +36,14 @@ public interface TableScan {
     /** Plan splits, throws {@link EndOfScanException} if the scan is ended. */
     Plan plan();
 
+    /**
+     * get partitions, this method wouldn't generate a full plan but only search for the partitions
+     * needed in the manifest files.
+     */
+    default List<Partition> partitions() {
+        return Collections.emptyList();

Review Comment:
   Why add a default implementation?



##########
paimon-core/src/main/java/org/apache/paimon/partition/PartitionBuilder.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.partition;
+
+import org.apache.paimon.data.BinaryRow;
+import org.apache.paimon.data.InternalRow;
+import org.apache.paimon.types.DataField;
+import org.apache.paimon.types.RowType;
+
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/** To build {@link Partition}. */
+public class PartitionBuilder {

Review Comment:
   You may just need one `RowDataPartitionComputer`?



##########
paimon-core/src/main/java/org/apache/paimon/partition/PartitionEntry.java:
##########
@@ -0,0 +1,115 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.partition;
+
+import org.apache.paimon.data.BinaryRow;
+import org.apache.paimon.manifest.FileKind;
+import org.apache.paimon.types.DataField;
+import org.apache.paimon.types.RowType;
+import org.apache.paimon.types.TinyIntType;
+import org.apache.paimon.utils.Preconditions;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import static org.apache.paimon.utils.SerializationUtils.newBytesType;
+import static org.apache.paimon.utils.SerializationUtils.newStringType;
+
+/** Partition Entry pojo read from manifest file. */
+public class PartitionEntry {
+    private final FileKind kind;
+
+    private final BinaryRow partition;
+
+    private final String fileName;
+
+    public FileKind kind() {
+        return kind;
+    }
+
+    public BinaryRow partition() {
+        return partition;
+    }
+
+    public String getFileName() {
+        return fileName;
+    }
+
+    public PartitionEntry(FileKind kind, BinaryRow partition, String fileName) {
+        this.kind = kind;
+        this.partition = partition;
+        this.fileName = fileName;
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+        if (!(obj instanceof PartitionEntry)) {
+            return false;
+        }
+        PartitionEntry eobj = (PartitionEntry) obj;
+
+        return Objects.equals(eobj.fileName, this.fileName)
+                && Objects.equals(eobj.partition, this.partition)
+                && Objects.equals(eobj.kind, this.kind);
+    }
+
+    public static RowType schema() {
+        List<DataField> fields = new ArrayList<>();
+        fields.add(new DataField(0, "_KIND", new TinyIntType(false)));
+        fields.add(new DataField(1, "_PARTITION", newBytesType(false)));
+        fields.add(new DataField(2, "_FILE", schemaDataFileName()));

Review Comment:
   `_FILE_NAME`?



##########
paimon-core/src/main/java/org/apache/paimon/partition/Partition.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.partition;

Review Comment:
   This can be in catalog package.



##########
paimon-core/src/main/java/org/apache/paimon/manifest/ManifestFile.java:
##########
@@ -183,5 +186,19 @@ public ManifestFile create() {
                     suggestedFileSize,
                     cache);
         }
+
+        // this reader read partition information by projection
+        public PartitionFileReader createPartitionFileReader() {

Review Comment:
   Just `PartitionReader`? This is manifest file, not partition file.



-- 
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: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org