[SPARK-33569][SQL] Remove getting partitions by an identifier prefix. #30514

MaxGekk · 2020-11-26T08:49:36Z

What changes were proposed in this pull request?

Remove the method listPartitionIdentifiers() from the SupportsPartitionManagement interface. The method lists partitions by ident prefix.
Rename listPartitionByNames() to listPartitionIdentifiers().
Re-implement the default method partitionExists() using new method.

Why are the changes needed?

Getting partitions by ident prefix only is not used, and it can be removed to improve code maintenance. Also this makes the SupportsPartitionManagement interface cleaner.

Does this PR introduce any user-facing change?

Should not.

How was this patch tested?

By running the affected test suites:

$ build/sbt "test:testOnly org.apache.spark.sql.connector.catalog.*"

SparkQA · 2020-11-26T13:04:11Z

Test build #131838 has finished for PR 30514 at commit 99cd1c2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jackylee-ch · 2020-11-26T17:04:49Z

...talyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsPartitionManagement.java

    /**
     * List the identifiers of all partitions that match to the ident by names.
     *
     * @param names the names of partition values in the identifier.
     * @param ident a partition identifier values.
     * @return an array of Identifiers for the partitions
     */
-    InternalRow[] listPartitionByNames(String[] names, InternalRow ident);
+    InternalRow[] listPartitionIdentifiers(String[] names, InternalRow ident);


How about listPartitionIdentifiers(StructType column, InternalRow ident)?

StructType requires to set additional fields like nullable, metadata and etc. that could be not necessary to list partitions. For example, I use this method in https://github.com/apache/spark/pull/30398/files#diff-b8fcff1b4de2d6e3641609dc02db791af8c56c2660123fa2b4afd408597fb401R44 where names are coming from ResolvedPartitionSpec. With our proposal, I will have to extract StructType from the partition schema before calling listPartitionIdentifiers which is not convenient ( and necessary) from my point of view.

jackylee-ch · 2020-11-26T17:17:23Z

...talyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsPartitionManagement.java

@@ -79,7 +80,9 @@ void createPartition(
     * @return true if the partition exists, false otherwise
     */
    default boolean partitionExists(InternalRow ident) {


This PR also changed this API. In the past, we coud use this API to check wether table had partitions

After the changes, you can do the same, right?

MaxGekk · 2020-11-27T11:04:00Z

@cloud-fan @HyukjinKwon @rdblue Could you review this PR, please.

cloud-fan · 2020-11-30T09:06:15Z

...talyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsPartitionManagement.java

@@ -79,7 +80,9 @@ void createPartition(
     * @return true if the partition exists, false otherwise
     */
    default boolean partitionExists(InternalRow ident) {
-        return listPartitionIdentifiers(ident).length > 0;
+        String[] partitionNames = partitionSchema().names();
+        String[] requiredNames = Arrays.copyOfRange(partitionNames, 0, ident.numFields());


shall we fail if the given ident is only a prefix? partitionExists sounds like it should be applied to a certain partition.

BTW which command needs it?

It uses in tests a lot, and in:

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/InMemoryAtomicPartitionTable.scala

Line 61 in 60fa8e3

if (idents.exists(partitionExists)) {

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/InMemoryAtomicPartitionTable.scala

Line 71 in 60fa8e3

if (!idents.forall(partitionExists)) {

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/AlterTableAddPartitionExec.scala

Line 40 in 1eb236b

partSpecs.partition(p => table.partitionExists(p.spec))

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/AlterTableDropPartitionExec.scala

Line 38 in 1eb236b

partSpecs.map(_.spec).partition(table.partitionExists)

I guess, we can rewrite the cases.

It seems like we always call it with a complete partition spec.

anyway it's not related to this PR.

cloud-fan · 2020-11-30T14:05:48Z

thanks, merging to master!

LuciferYang · 2020-11-30T15:55:41Z

[error] /home/runner/work/spark/spark/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowPartitionsExec.scala:44:38: value listPartitionByNames is not a member of org.apache.spark.sql.connector.catalog.SupportsPartitionManagement
[error]     val partitionIdentifiers = table.listPartitionByNames(names.toArray, ident)
[error]

Are there missing codes? It seems that the latest code can't be compiled @cloud-fan @MaxGekk

MaxGekk · 2020-11-30T16:17:29Z

Thank you, @LuciferYang . I am about to open a PR with a fix.

MaxGekk · 2020-11-30T16:28:18Z

Here is the fix for the build error: #30553

…artitionsExec` ### What changes were proposed in this pull request? Use `listPartitionIdentifiers ` instead of `listPartitionByNames` in `ShowPartitionsExec`. The `listPartitionByNames` was renamed by #30514. ### Why are the changes needed? To fix build error. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By running tests for the `SHOW PARTITIONS` command: ``` $ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *ShowPartitionsSuite" ``` Closes #30553 from MaxGekk/fix-build-show-partitions-exec. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…artitionsExec` ### What changes were proposed in this pull request? Use `listPartitionIdentifiers ` instead of `listPartitionByNames` in `ShowPartitionsExec`. The `listPartitionByNames` was renamed by apache/spark#30514. ### Why are the changes needed? To fix build error. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By running tests for the `SHOW PARTITIONS` command: ``` $ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *ShowPartitionsSuite" ``` Closes #30553 from MaxGekk/fix-build-show-partitions-exec. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

MaxGekk added 3 commits November 26, 2020 11:09

Re-implement partitionExists()

a23b014

Remove listPartitionIdentifiers()

29519bb

listPartitionByNames() -> listPartitionIdentifiers()

99cd1c2

github-actions bot added the SQL label Nov 26, 2020

MaxGekk changed the title ~~[SPARK-33569][SQL] Remove getting partitions by only identifiers~~ [SPARK-33569][SQL] Remove getting partitions by an identifier prefix. Nov 26, 2020

MaxGekk mentioned this pull request Nov 26, 2020

[SPARK-33509][SQL] List partition by names from a V2 table which supports partition management #30452

Closed

jackylee-ch reviewed Nov 26, 2020

View reviewed changes

cloud-fan reviewed Nov 30, 2020

View reviewed changes

cloud-fan approved these changes Nov 30, 2020

View reviewed changes

cloud-fan closed this in 6fd148f Nov 30, 2020

MaxGekk mentioned this pull request Nov 30, 2020

[SPARK-33569][SPARK-33452][SQL][FOLLOWUP] Fix a build error in ShowPartitionsExec #30553

Closed

MaxGekk deleted the remove-listPartitionIdentifiers branch February 19, 2021 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-33569][SQL] Remove getting partitions by an identifier prefix. #30514

[SPARK-33569][SQL] Remove getting partitions by an identifier prefix. #30514

MaxGekk commented Nov 26, 2020

SparkQA commented Nov 26, 2020

jackylee-ch Nov 26, 2020

MaxGekk Nov 26, 2020

jackylee-ch Nov 26, 2020

MaxGekk Nov 26, 2020

MaxGekk commented Nov 27, 2020 •

edited

Loading

cloud-fan Nov 30, 2020

MaxGekk Nov 30, 2020

cloud-fan Nov 30, 2020

cloud-fan Nov 30, 2020

cloud-fan commented Nov 30, 2020

LuciferYang commented Nov 30, 2020

MaxGekk commented Nov 30, 2020

MaxGekk commented Nov 30, 2020

[SPARK-33569][SQL] Remove getting partitions by an identifier prefix. #30514

[SPARK-33569][SQL] Remove getting partitions by an identifier prefix. #30514

Conversation

MaxGekk commented Nov 26, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Nov 30, 2020

LuciferYang commented Nov 30, 2020

MaxGekk commented Nov 30, 2020

MaxGekk commented Nov 30, 2020

MaxGekk commented Nov 27, 2020 •

edited

Loading