[GLUTEN-3378][CORE] Move getLocalFilesNode logic to scan transformer #3650

liujiayi771 · 2023-11-08T09:42:19Z

What changes were proposed in this pull request?

The Lake format has its own custom partition type, with a processing logic that is different from the existing FilePartition and GlutenMergeTreePartition. In the unified design, the Lake format will have its own scan transformer. In order to ensure that the injection of the Lake format does not affect gluten-core, the process of generating LocalFilesNode from handling partitions needs to be moved to the scan transformer, allowing the Lake format's scan transformer to customize the processing logic of the partitions.

Introduce ReadSplit interface for ExtensionTableNode and LocalFileNode, so that we can get rid of using java.io.Serializable.

How was this patch tested?

Exists CI.

github-actions · 2023-11-08T09:42:38Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Other pull requests

github-actions · 2023-11-08T09:42:53Z

Run Gluten Clickhouse CI

github-actions · 2023-11-08T12:20:35Z

Run Gluten Clickhouse CI

github-actions · 2023-11-08T14:07:01Z

Run Gluten Clickhouse CI

github-actions · 2023-11-08T14:29:29Z

Run Gluten Clickhouse CI

github-actions · 2023-11-08T14:58:43Z

Run Gluten Clickhouse CI

github-actions · 2023-11-09T02:23:28Z

Run Gluten Clickhouse CI

YannByron · 2023-11-09T09:08:13Z

gluten-core/src/main/scala/io/glutenproject/execution/WholeStageTransformer.scala

-      if (allScanPartitions.exists(_.size != partitionLength)) {
+      val allScanLocalFilesNodes = basicScanExecTransformers.map(_.getLocalFilesNodes)
+      val partitionLength = allScanLocalFilesNodes.head.size
+      if (allScanLocalFilesNodes.exists(_.size != partitionLength)) {


maybe extract a private method to encapsulate this logic that contains getLocalFilesNodes, the assertion of whether the number of partitions is consistent, and the transposition. And doc for this method.

YannByron · 2023-11-09T09:18:02Z

gluten-core/src/main/scala/io/glutenproject/execution/BasicScanExecTransformer.scala

@@ -54,6 +54,11 @@ trait BasicScanExecTransformer extends LeafTransformSupport with SupportFormat {
  // TODO: Remove this expensive call when CH support scan custom partition location.
  def getInputFilePaths: Seq[String]

+  def getLocalFilesNodes: Seq[(java.io.Serializable, Array[String])] =


can we define a case class for (java.io.Serializable, Array[String])? To make Serializable as return type is so wired.

This is because CK is using ExtensionTable instead of LocalFilesNode, and their common superclass is Serializable. Now, a new interface called ReadSplit is introduced to represent the abstraction of both classes. Additionally, preferredLocations have been included in the class to avoid using Tuple.

YannByron · 2023-11-09T09:21:54Z

gluten-core/src/main/scala/org/apache/spark/softaffinity/SoftAffinityUtil.scala

-
+  def getFilePartitionLocations(
+      filePaths: Array[String],
+      preferredLocations: Array[String]): Array[String] = {


can you explain this definition a little?

Previously, the input of this method was FilePartition, but the InputPartitions for lake formats are custom-defined. This method primarily processes FilePartition.files.filePath and preferredLocations. Modifying the input type of this method would allow the lake formats to directly reuse this interface.

github-actions · 2023-11-09T15:37:10Z

Run Gluten Clickhouse CI

github-actions · 2023-11-10T02:29:49Z

Run Gluten Clickhouse CI

liujiayi771 · 2023-11-10T06:04:51Z

@yma11 @zhztheplayer Could you help review these changes?

github-actions · 2023-11-10T23:47:48Z

Run Gluten Clickhouse CI

liujiayi771 · 2023-11-13T02:31:53Z

@zzcclp Could you help review this PR?

yma11 · 2023-11-13T23:58:15Z

gluten-core/src/main/java/io/glutenproject/substrait/rel/LocalFilesNode.java

@@ -30,12 +30,13 @@
 import java.util.List;
 import java.util.Map;

-public class LocalFilesNode implements Serializable {
+public class LocalFilesNode implements ReadSplit, Serializable {


Can we have a better name instead of ReadSplit which can show the inheritance relationship here? maybe FilesNode if there is kind of IceBergFilesNode / DeltaFilesNode exist?

We have also spent time thinking about the name of this interface. It mainly serves as an abstract interface for ExtensionTableNode and LocalFilesNode. In the future, both Iceberg and Delta will use LocalFilesNode, and there will be no IcebergFilesNode. We think that LocalFiles are split meant for handling by the underlying engine.

I see. Then maybe SplitInfo is more suitable in this case and it has corresponding definition like "SplitInfo" or HiveConnectorSplit at velox side.

@yma11 I think it makes sense. I will change it to SplitInfo.

yma11 · 2023-11-14T00:00:00Z

gluten-core/src/main/scala/io/glutenproject/execution/WholeStageTransformer.scala

+
+  private def getReadSplitFromScanTransformer(
+      basicScanExecTransformers: Seq[BasicScanExecTransformer]): Seq[Seq[ReadSplit]] = {
+    // If these are two scan transformers, they must have same partitions,


When will there are multi scan transformers? Will they be against same table?

You can check this test VeloxTPCHV1BhjSuite, it will trigger multi scan transformer. This situation can occur with a broadcast join.

I do not thinkg bhj have two scan transformers in one stage, the build side should have a broadcast exchange. Bucket table join is a valid case which contain two scan transformers in one stage.

yma11 · 2023-11-14T00:16:26Z

@rui-mo can you also take a review on this PR as it has some substrait module related changes?

rui-mo · 2023-11-14T03:24:29Z

gluten-core/src/main/java/io/glutenproject/substrait/rel/ReadSplit.java

+import java.util.List;
+
+public interface ReadSplit {
+  List<String> preferredLocations();


Maybe add some comments for this class and the member function preferredLocations.

Add some comments. Do you think the name ReadSplit needs to be modified?

github-actions · 2023-11-14T11:08:41Z

Run Gluten Clickhouse CI

github-actions · 2023-11-14T14:30:55Z

Run Gluten Clickhouse CI

liujiayi771 · 2023-11-10T02:40:49Z

gluten-core/src/main/scala/io/glutenproject/execution/WholeStageTransformer.scala

@@ -336,6 +330,32 @@ case class WholeStageTransformer(child: SparkPlan, materializeInput: Boolean = f

  override protected def withNewChildInternal(newChild: SparkPlan): WholeStageTransformer =
    copy(child = newChild, materializeInput = materializeInput)(transformStageId)
+
+  private def getReadSplitFromScanTransformer(


@YannByron Add this private function.

liujiayi771 · 2023-11-14T01:33:36Z

gluten-core/src/main/java/io/glutenproject/substrait/rel/LocalFilesNode.java

@@ -30,12 +30,13 @@
 import java.util.List;
 import java.util.Map;

-public class LocalFilesNode implements Serializable {
+public class LocalFilesNode implements ReadSplit, Serializable {


We have also spent time thinking about the name of this interface. It mainly serves as an abstract interface for ExtensionTableNode and LocalFilesNode. In the future, both Iceberg and Delta will use LocalFilesNode, and there will be no IcebergFilesNode. We think that LocalFiles are split meant for handling by the underlying engine.

liujiayi771 · 2023-11-14T01:35:45Z

gluten-core/src/main/scala/io/glutenproject/execution/WholeStageTransformer.scala

+
+  private def getReadSplitFromScanTransformer(
+      basicScanExecTransformers: Seq[BasicScanExecTransformer]): Seq[Seq[ReadSplit]] = {
+    // If these are two scan transformers, they must have same partitions,


You can check this test VeloxTPCHV1BhjSuite, it will trigger multi scan transformer. This situation can occur with a broadcast join.

liujiayi771 · 2023-11-15T03:02:07Z

gluten-core/src/main/java/io/glutenproject/substrait/rel/ReadSplit.java

+import java.util.List;
+
+public interface ReadSplit {
+  List<String> preferredLocations();


Add some comments. Do you think the name ReadSplit needs to be modified?

github-actions · 2023-11-16T14:07:18Z

Run Gluten Clickhouse CI

github-actions · 2023-11-16T14:57:19Z

#3378

github-actions · 2023-11-17T02:13:05Z

Run Gluten Clickhouse CI

github-actions · 2023-11-17T03:26:06Z

Run Gluten Clickhouse CI

ulysses-you · 2023-11-17T03:29:51Z

gluten-core/src/main/scala/io/glutenproject/execution/WholeStageTransformer.scala

+            GlutenPartition(
+              index,
+              substraitPlan.toByteArray,
+              splitInfos.head.preferredLocations().asScala.toArray)


splitInfos(i).preferredLocations ?

@ulysses-you Currently, the GlutenPartition only has one preferredLocations. However, within GlutenPartition, there are multiple SplitInfo instances, and each SplitInfo may have a different preferredLocations. However, the existing code only takes the first preferredLocations.

https://github.com/oap-project/gluten/blob/f21a96f727ab8368d1de0de25101b41c61d58038/backends-velox/src/main/scala/io/glutenproject/backendsapi/velox/IteratorApiImpl.scala#L126

I see, it's for one partition but for several scans. I think the currently implementation loses some of scan preferredLocations. Shall we use splitInfos.flatMap(_.preferredLocations).distinct ?

Yes. Merge all preferredLocations is better. I will fix.

Additional note: distinct is not need as Spark will count the locations to determine which locations are preferred.

ulysses-you · 2023-11-17T03:32:41Z

backends-velox/src/main/scala/io/glutenproject/backendsapi/velox/IteratorApiImpl.scala

+          partitionColumns,
+          fileFormat,
+          preferredLocations.toList.asJava)
+    }


nit: throw exception for default case

github-actions · 2023-11-17T04:09:08Z

Run Gluten Clickhouse CI

github-actions · 2023-11-17T04:26:56Z

Run Gluten Clickhouse CI

github-actions · 2023-11-17T05:21:28Z

Run Gluten Clickhouse CI

GlutenPerfBot · 2023-11-17T08:45:11Z

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query	log/native_3650_time.csv	log/native_master_11_16_2023_510b22841_time.csv	difference	percentage
q1	34.28	34.35	0.068	100.20%
q2	24.85	24.60	-0.248	99.00%
q3	37.51	37.34	-0.176	99.53%
q4	36.56	37.04	0.475	101.30%
q5	70.17	68.82	-1.345	98.08%
q6	7.02	7.03	0.011	100.16%
q7	84.37	85.40	1.030	101.22%
q8	85.40	85.86	0.462	100.54%
q9	123.05	126.12	3.067	102.49%
q10	44.48	46.82	2.338	105.26%
q11	20.89	19.68	-1.210	94.21%
q12	25.00	24.50	-0.498	98.01%
q13	45.97	46.88	0.919	102.00%
q14	17.37	18.78	1.411	108.12%
q15	28.78	28.03	-0.754	97.38%
q16	15.08	15.56	0.478	103.17%
q17	101.64	99.58	-2.062	97.97%
q18	146.70	148.00	1.300	100.89%
q19	12.96	12.89	-0.071	99.45%
q20	28.64	28.05	-0.589	97.94%
q21	219.86	220.92	1.060	100.48%
q22	12.95	12.73	-0.214	98.35%
total	1223.52	1228.97	5.452	100.45%

liujiayi771 force-pushed the scan-part branch from a745dee to aa46284 Compare November 8, 2023 12:19

liujiayi771 force-pushed the scan-part branch from aa46284 to 852596e Compare November 8, 2023 14:06

liujiayi771 force-pushed the scan-part branch from 852596e to 9ac0137 Compare November 8, 2023 14:28

liujiayi771 force-pushed the scan-part branch from 9ac0137 to ef5dd5a Compare November 8, 2023 14:58

liujiayi771 force-pushed the scan-part branch from ef5dd5a to 8ece5c5 Compare November 9, 2023 02:22

YannByron reviewed Nov 9, 2023

View reviewed changes

liujiayi771 force-pushed the scan-part branch from fd2159f to 4460e2f Compare November 10, 2023 23:47

liujiayi771 marked this pull request as ready for review November 13, 2023 02:31

yma11 reviewed Nov 13, 2023

View reviewed changes

yma11 reviewed Nov 14, 2023

View reviewed changes

rui-mo requested a review from zzcclp November 14, 2023 03:18

rui-mo reviewed Nov 14, 2023

View reviewed changes

liujiayi771 force-pushed the scan-part branch from 71a7480 to cbafdc6 Compare November 14, 2023 14:30

liujiayi771 changed the title ~~[WIP][CORE] Move getLocalFilesNode logic to scan transformer~~ [CORE] Move getLocalFilesNode logic to scan transformer Nov 16, 2023

liujiayi771 commented Nov 16, 2023

View reviewed changes

liujiayi771 added 2 commits November 16, 2023 22:06

move getLocalFilesNode logic to transformer

e58cf93

add some comments in ReadSplit

5ef217e

liujiayi771 force-pushed the scan-part branch from cbafdc6 to 5ef217e Compare November 16, 2023 14:06

liujiayi771 changed the title ~~[CORE] Move getLocalFilesNode logic to scan transformer~~ [GLUTEN-3378][CORE] Move getLocalFilesNode logic to scan transformer Nov 16, 2023

rename ReadSplit to SplitInfo

b87c8e3

fix style

c5202d2

ulysses-you reviewed Nov 17, 2023

View reviewed changes

Add default branch in velox genSplitInfo

e73576e

merge all preferredLocations in GlutenPartition

e505eb1

remove distinct

b0aba66

ulysses-you approved these changes Nov 17, 2023

View reviewed changes

yma11 merged commit 49b8e06 into apache:main Nov 17, 2023
17 checks passed

liujiayi771 deleted the scan-part branch November 17, 2023 08:11

rz-vastdata mentioned this pull request Nov 21, 2023

[VL] Unified design for data lake read support in Gluten + Velox #3378

Open

[GLUTEN-3378][CORE] Move getLocalFilesNode logic to scan transformer #3650

[GLUTEN-3378][CORE] Move getLocalFilesNode logic to scan transformer #3650

Conversation

liujiayi771 commented Nov 8, 2023 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 9, 2023

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

Choose a reason for hiding this comment

github-actions bot commented Nov 9, 2023

github-actions bot commented Nov 10, 2023

liujiayi771 commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

liujiayi771 commented Nov 13, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yma11 commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2023

github-actions bot commented Nov 14, 2023

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

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 16, 2023

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

Choose a reason for hiding this comment

liujiayi771 Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liujiayi771 Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

GlutenPerfBot commented Nov 17, 2023

liujiayi771 commented Nov 8, 2023 •

edited

Loading

liujiayi771 Nov 17, 2023 •

edited

Loading

liujiayi771 Nov 17, 2023 •

edited

Loading