You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "ivoson (via GitHub)" <gi...@apache.org> on 2024/10/15 03:08:53 UTC
[PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
ivoson opened a new pull request, #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533
## What changes were proposed in this pull request?
Rewrite the `TakeOrderedAndProjectExecTransformer` to the operators which are actually executed when do offload. Currently, the logic is wrapped in the `RDD` of the operator. Which means that in the RDD computation we play with the sql operators to generate new `RDD`s which is not a common practice processing sql queries. And the final executed plan and metrics information won't be shown correctly in the Spark UI page.
In this PR, rewrite the node `TakeOrderedAndProjectExecTransformer` when do offload so that the operators can be shown correctly.
## How was this patch tested?
UT
Here is an example of the plan changes before and after the change for query:
```
```
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2413364898
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1809770192
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
> Is this inevitable to add 'OutputPartitioning' for implementing this feature?
I think so. We need the partitioning information to decide whether a shuffle is needed. Otherwise, we can only add the shuffle blindly and then add another rule to remove unnecessary ones afterwards somewhere else.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2412773648
+1 to the idea. I remember there was some vanilla Spark limitation as blocker of an similar effort. But let's see if it's doable at this time.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject as actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2543411030
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1808308268
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
Speaking of query optimizer, ideally `OutputPartitioning` should be categorized into a physical [`Property`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/PropertyModel.scala) than into [`Metadata`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/MetadataModel.scala).
But the implementation can be trickier. Is this inevitable to add this code?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2421087992
cc @zhztheplayer can you please review this PR? Thanks.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2412753025
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2419012174
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject as actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2437291664
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject as actually executed operators when do offload [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1821964317
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
> Or I think we can keep the PR open and take some time to look back on [1] to see if there are still blockers against the effort.
Hi @zhztheplayer , do you mean any blockers to incorporate OutputPartitioning / OutputOrdering into RAS planner? Anything I can help here?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1808308268
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
Speaking of query optimizer, ideally `OutputPartitioning` could be categorized as a physical [`Property`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/PropertyModel.scala) than as [`Metadata`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/MetadataModel.scala).
But the implementation can be trickier. Is this inevitable to add this code for implementing this feature?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1808308268
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
Speaking of query optimizer, ideally `OutputPartitioning` could be categorized as a physical [`Property`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/PropertyModel.scala) than into [`Metadata`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/MetadataModel.scala).
But the implementation can be trickier. Is this inevitable to add this code?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1808308268
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
Speaking of query optimizer, ideally `OutputPartitioning` could be categorized as a physical [`Property`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/PropertyModel.scala) than as [`Metadata`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/MetadataModel.scala). But the implementation can be trickier.
Is this inevitable to add 'OutputPartitioning' for implementing this feature?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject as actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2562070713
This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2412752065
https://github.com/apache/incubator-gluten/issues/7532
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject as actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1818305756
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
@ivoson There was an old attempt from my side for incorporating `OutputPartitioning` / `OutputOrdering` into RAS planner.
[1] https://github.com/zhztheplayer/gluten/commit/d1575cce3626eaf4efe8c06924d108008e6e2c1e#diff-305f3a48d262389cc311d17f15cd27f6a1efc0d6da2dcd0ebe75975ccf2e9447
This PR's solution is OK to me as of now, but I think we can do a refactor in future to use properties rather than metadata.
Or I think we can keep the PR open and take some time to look back on `[1]` to see if there are still blockers against the effort. I feel we should make sure the refactor will be viable then it's safe to merge the current PR's code as a temporary solution.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1808308268
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
Speaking of query optimizer, ideally `OutputPartitioning` could be categorized as a physical [`Property`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/PropertyModel.scala) than into [`Metadata`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/MetadataModel.scala).
But the implementation can be trickier. Is this inevitable to add this code for implementing this feature?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject as actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #7533: [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject as actually executed operators when do offload
URL: https://github.com/apache/incubator-gluten/pull/7533
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2416210719
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#issuecomment-2419040815
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1808308268
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
Speaking of query optimizer, ideally `OutputPartitioning` could be categorized as a physical [`Property`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/PropertyModel.scala) than as [`Metadata`](https://github.com/apache/incubator-gluten/blob/main/gluten-ras/common/src/main/scala/org/apache/gluten/ras/MetadataModel.scala).
But the implementation can be trickier. Is this inevitable to add 'OutputPartitioning' for implementing this feature?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7532][CORE] Rewrite TakeOrderedAndProject with actually executed operators when do offload [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #7533:
URL: https://github.com/apache/incubator-gluten/pull/7533#discussion_r1809953503
##########
gluten-substrait/src/main/scala/org/apache/gluten/planner/metadata/OutputPartitioning.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.gluten.planner.metadata
+
+import org.apache.gluten.planner.metadata.GlutenMetadataModel.Verifier
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.sql.catalyst.plans.physical.Partitioning
+
+case class OutputPartitioning(partitioning: Partitioning) {
+ override def hashCode(): Int = System.identityHashCode(partitioning)
+ override def equals(obj: Any): Boolean = obj match {
+ case OutputPartitioning(otherPartitioning) => partitioning.equals(otherPartitioning)
+ case _ => false
+ }
+
+ override def toString: String = s"$partitioning"
+}
Review Comment:
Thank you for the explanation. And I'll do some local tests with the change until this weekend then get back to you.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org