You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/07 09:56:00 UTC
[GitHub] [arrow] jvanstraten opened a new pull request, #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
jvanstraten opened a new pull request, #13537:
URL: https://github.com/apache/arrow/pull/13537
This introduces the conversion options structure described in the associated JIRA. In the interest of keeping this small, no behavior has been modified to make use of the options yet; the newly added options can only be used to relax the conversion semantics, so the current implementation already satisfies all of them. I'll submit followup issues for features that can make use of the relaxed semantics.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920302519
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
Review Comment:
```suggestion
/// will round-trip. Relations/nodes must map one-to-one.
///
/// For example, any "hint" that Acero does not recognize will cause
/// conversion to fail.
```
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
Review Comment:
```suggestion
/// expression may be emulated using a project-order-project triple.
```
I'm not sure this last sentence is needed. I could imagine a scenario might exist where this would result in a many-to-one as well (or one to zero) if a node is deemed supplementary.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r923849990
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, reject the conversion. This effectively asserts that there is no (known)
+ /// information loss in the conversion, and that plans should either round-trip back and
+ /// forth exactly or not at all. This option is primarily intended for testing and
+ /// debugging.
+ EXACT_ROUNDTRIP,
+
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, attempt to model it with some collection of primitives at the output. This
+ /// means that even if the incoming plan is completely optimal by some metric, the
+ /// returned plan is fairly likely to not be optimal anymore, and round-trips back and
+ /// forth may make the plan increasingly suboptimal. However, every primitive at the
+ /// output can be (manually) traced back to exactly one primitive at the input, which
+ /// may be useful when debugging.
+ PRESERVE_STRUCTURE,
+
+ /// Behaves like PRESERVE_STRUCTURE, but prefers performance over structural accuracy.
+ /// Basic optimizations *may* be applied, in order to attempt to not regress in terms of
+ /// plan performance: if the incoming plan was already aggressively optimized, the goal
+ /// is for the output plan to not be less performant. In practical use cases, this is
+ /// probably the option you want.
+ ///
+ /// Note that no guarantees are made on top of PRESERVE_STRUCTURE. Past and future
+ /// versions of Arrow may even ignore this option entirely and treat it exactly like
+ /// PRESERVE_STRUCTURE.
+ BEST_EFFORT,
+};
+
+/// Controls how to convert between Substrait and Acero representations of a plan.
+struct ConversionOptions {
+ /// Controls how strictly the converter is to adhere to the structure of the input.
+ ConversionStrictness strictness = ConversionStrictness::EXACT_ROUNDTRIP;
Review Comment:
Okay, done in b91cd77
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r923846240
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, reject the conversion. This effectively asserts that there is no (known)
+ /// information loss in the conversion, and that plans should either round-trip back and
+ /// forth exactly or not at all. This option is primarily intended for testing and
+ /// debugging.
+ EXACT_ROUNDTRIP,
+
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, attempt to model it with some collection of primitives at the output. This
+ /// means that even if the incoming plan is completely optimal by some metric, the
+ /// returned plan is fairly likely to not be optimal anymore, and round-trips back and
+ /// forth may make the plan increasingly suboptimal. However, every primitive at the
+ /// output can be (manually) traced back to exactly one primitive at the input, which
+ /// may be useful when debugging.
+ PRESERVE_STRUCTURE,
+
+ /// Behaves like PRESERVE_STRUCTURE, but prefers performance over structural accuracy.
+ /// Basic optimizations *may* be applied, in order to attempt to not regress in terms of
+ /// plan performance: if the incoming plan was already aggressively optimized, the goal
+ /// is for the output plan to not be less performant. In practical use cases, this is
+ /// probably the option you want.
+ ///
+ /// Note that no guarantees are made on top of PRESERVE_STRUCTURE. Past and future
+ /// versions of Arrow may even ignore this option entirely and treat it exactly like
+ /// PRESERVE_STRUCTURE.
+ BEST_EFFORT,
+};
+
+/// Controls how to convert between Substrait and Acero representations of a plan.
+struct ConversionOptions {
+ /// Controls how strictly the converter is to adhere to the structure of the input.
+ ConversionStrictness strictness = ConversionStrictness::EXACT_ROUNDTRIP;
Review Comment:
I think we should change it. Substrait is not only experimental but, TMK, we haven't actually released any builds with advertised Substrait features so there is no concern yet for breaking changes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1181770186
There is a merge conflict, can you rebase and resolve it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920246357
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
Review Comment:
"Pedantic" conveys an idea of spurious or arbitrary constraints. Is this the case here? Otherwise "strict" would sound better IMHO.
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
Review Comment:
```suggestion
/// may thus map one-to-many.
```
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
+ PRESERVE_STRUCTURE,
+
+ /// Attempt to prevent performance-related regressions caused by differences in how
+ /// operations are represented in the input and output format, by allowing for
+ /// optimizations that cross structural boundaries. For example, the converter may
+ /// collapse chains of project nodes into one.
Review Comment:
A more general question is: is BEST_EFFORT always more lenient than PRESERVE_STRUCTURE?
In PRESERVE_STRUCTURE I see "Relations/nodes will thus map one-to-many", does BEST_EFFORT also allow the converse (relations/nodes mapping many-to-one)?
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
+ PRESERVE_STRUCTURE,
+
+ /// Attempt to prevent performance-related regressions caused by differences in how
+ /// operations are represented in the input and output format, by allowing for
+ /// optimizations that cross structural boundaries. For example, the converter may
+ /// collapse chains of project nodes into one.
Review Comment:
Isn't collapsing of a chain of project nodes more of an optimization than conversion concern? The collapsed node should be strictly equivalent to the original chain AFAIU.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1198321767
Done.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920445140
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
Review Comment:
Sure, that probably captures it better.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1183610872
> First, Is there a need/ask for PRESERVE_STRUCTURE?
I have to admit I'm having a hard time coming up with a use case for it right now. It just feels right to keep it in there. By analogy with compiler flags:
- PEDANTIC is like `gcc -Werror -Wall -pedantic-errors`: "if there is even the slightest hint of something maybe being wrong, please scream bloody murder."
- PRESERVE_STRUCTURE is like `gcc -g -O0`: "if I write something stupid, please keep it exactly equally stupid, because I'm trying to figure out why my stuff isn't working." This doesn't really apply here (yet) because Substrait and (presumably) Acero both lack a means to annotate nodes with debug information, but I could still see it be useful when a user is debugging a query that somehow passes through Arrow.
- BEST_EFFORT is like `gcc -O3` (or perhaps more accurately just `gcc` because we're not involving a proper optimization engine): "I just want my code/plan to work, and preferably work fast. Don't even tell me if I did something stupid, because I'm not interested in that right now."
> Second, should the ExtensionSet be made an argument of conversion options?
Maybe. It's part of the reason why I stuffed the enum in a struct. However, it's worth noting that ExtensionSet is mutated by the conversion, so it's not strictly speaking just an option.
> Alternatively, we could bite the bullet and make the Substrait conversion object oriented.
@vibhatha and I discussed this offline for a bit and agree that this would be better. See ARROW-16987. That would be a longer-term thing though, so I submitted this to not have the logic for these things be blocked by a possible refactoring later.
---
I'll address the suggested changes to comments and the likes when we reach consensus about the options themselves.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r923739152
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, reject the conversion. This effectively asserts that there is no (known)
+ /// information loss in the conversion, and that plans should either round-trip back and
+ /// forth exactly or not at all. This option is primarily intended for testing and
+ /// debugging.
+ EXACT_ROUNDTRIP,
+
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, attempt to model it with some collection of primitives at the output. This
+ /// means that even if the incoming plan is completely optimal by some metric, the
+ /// returned plan is fairly likely to not be optimal anymore, and round-trips back and
+ /// forth may make the plan increasingly suboptimal. However, every primitive at the
+ /// output can be (manually) traced back to exactly one primitive at the input, which
+ /// may be useful when debugging.
+ PRESERVE_STRUCTURE,
+
+ /// Behaves like PRESERVE_STRUCTURE, but prefers performance over structural accuracy.
+ /// Basic optimizations *may* be applied, in order to attempt to not regress in terms of
+ /// plan performance: if the incoming plan was already aggressively optimized, the goal
+ /// is for the output plan to not be less performant. In practical use cases, this is
+ /// probably the option you want.
+ ///
+ /// Note that no guarantees are made on top of PRESERVE_STRUCTURE. Past and future
+ /// versions of Arrow may even ignore this option entirely and treat it exactly like
+ /// PRESERVE_STRUCTURE.
+ BEST_EFFORT,
+};
+
+/// Controls how to convert between Substrait and Acero representations of a plan.
+struct ConversionOptions {
+ /// Controls how strictly the converter is to adhere to the structure of the input.
+ ConversionStrictness strictness = ConversionStrictness::EXACT_ROUNDTRIP;
Review Comment:
Heh, that's a good point. I set it that way because that's how the logic has behaved up to this point, so in some sense it feels like a breaking change to set the default to anything else. I suppose the Substrait interface is marked as experimental, though. Do you think I should change it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920445549
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
+ PRESERVE_STRUCTURE,
+
+ /// Attempt to prevent performance-related regressions caused by differences in how
+ /// operations are represented in the input and output format, by allowing for
+ /// optimizations that cross structural boundaries. For example, the converter may
+ /// collapse chains of project nodes into one.
Review Comment:
Got it, I'll reformulate. :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920406800
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
Review Comment:
> "Pedantic" conveys an idea of spurious or arbitrary constraints. Is this the case here?
I'd say so, actually. "Pedantic" includes constraints such as rejecting an Acero plan for conversion to Substrait if the internal column names are not exactly what you'd get after round-tripping through Substrait (which cannot represent inner column names), even though the plan can be conveyed in Substrait perfectly well.
> Otherwise "strict" would sound better IMHO.
Strict implies to me to strictly conform to all the rules of the output format, i.e. don't make anything that's out of spec. But none of the options do that, so it feels kind of meaningless.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920427455
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
Review Comment:
Hmm... perhaps "exact_roundtrip" or "perfect_roundtrip" based on what I'm reading below?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920510226
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
Review Comment:
Looking through the current nodes I can't come up with a good concrete example. Perhaps something like a sort node. We don't support mid-plan sort in Acero today (because we can't maintain the ordering after the node) but we do support sorting sinks (because there is no "after the node"). So it's possible we could have a RunSubstraitSorted function that allows plans that end in a sort node even though the sort node itself wouldn't be in the actual plan that gets run (just the sink would be sorted).
It's a bit of a stretch though. Either way, I think this is more of an implementation detail and not something to concern users.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1197682383
@jvanstraten Can you rebase this? I think there are a few spots that that snuck in that still need `, conversion_options` added.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1183440299
Travis-CI issues on s390x look unrelated.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1183680809
> PRESERVE_STRUCTURE is like gcc -g -O0: "if I write something stupid, please keep it exactly equally stupid, because I'm trying to figure out why my stuff isn't working." This doesn't really apply here (yet) because Substrait and (presumably) Acero both lack a means to annotate nodes with debug information, but I could still see it be useful when a user is debugging a query that somehow passes through Arrow.
This is probably a workable explanation. My primary concern is that I want a user to easily know which option to pick. Can we update the description with something like "This may be useful if you are debugging plan execution and want to avoid optimizations that may restructure the plan."
> Maybe. It's part of the reason why I stuffed the enum in a struct. However, it's worth noting that ExtensionSet is mutated by the conversion, so it's not strictly speaking just an option.
We have some precedence for this and signal it by passing the argument via pointer. For example, `SinkNodeOptions::generator`. However, in those cases we are usually pointing to "infrastructure". I understand your reasoning. I am not terribly fond of "options" objects that become "context" objects.
> @vibhatha and I discussed this offline for a bit and agree that this would be better. See [ARROW-16987](https://issues.apache.org/jira/browse/ARROW-16987). That would be a longer-term thing though, so I submitted this to not have the logic for these things be blocked by a possible refactoring later.
If it's going to be refactored later then I'm fine with this for now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920445549
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
+ PRESERVE_STRUCTURE,
+
+ /// Attempt to prevent performance-related regressions caused by differences in how
+ /// operations are represented in the input and output format, by allowing for
+ /// optimizations that cross structural boundaries. For example, the converter may
+ /// collapse chains of project nodes into one.
Review Comment:
Got it, I'll reformulate. :) (probably tomorrow though)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920428105
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
+ PRESERVE_STRUCTURE,
+
+ /// Attempt to prevent performance-related regressions caused by differences in how
+ /// operations are represented in the input and output format, by allowing for
+ /// optimizations that cross structural boundaries. For example, the converter may
+ /// collapse chains of project nodes into one.
Review Comment:
Ok, I think the BEST_EFFORT description you gave just above is clearer than in the PR :-)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920428039
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
Review Comment:
I'm not sure what you mean by "supplementary node." Can you give an example?
If you mean for example a no-op project node, my intention for PRESERVE_STRUCTURE is to keep that no-op project node in there. Removing it would be an optimization-esque thing and thus belongs in BEST_EFFORT.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1198878296
Benchmark runs are scheduled for baseline = 96679463056e9985b8ff745fb2273f71338bd864 and contender = 9a00777974df8c69921b0c08e80b5e6e842f41e9. 9a00777974df8c69921b0c08e80b5e6e842f41e9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f29a8a1a683341bea99764148ca0454b...5bbdf17808c541ada602659d1610bb6c/)
[Finished :arrow_down:0.34% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/0ce4ff3cd0734ed6ba860438ac1f935e...c59b92762b60409693d217fb4f652d59/)
[Finished :arrow_down:0.27% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7c414c6b3c344ecd8548d58aa25d5dc4...03143fc045a84792831820c88acc23c7/)
[Finished :arrow_down:0.07% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/4ba33be1927c4d2284ccc960003ad3e2...6b67e86a6e4f4b329cbebcdc93249265/)
Buildkite builds:
[Failed] [`9a007779` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1220)
[Finished] [`9a007779` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1232)
[Finished] [`9a007779` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1214)
[Finished] [`9a007779` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1234)
[Failed] [`96679463` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1219)
[Finished] [`96679463` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1231)
[Finished] [`96679463` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1213)
[Finished] [`96679463` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1233)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r923710217
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, reject the conversion. This effectively asserts that there is no (known)
+ /// information loss in the conversion, and that plans should either round-trip back and
+ /// forth exactly or not at all. This option is primarily intended for testing and
+ /// debugging.
+ EXACT_ROUNDTRIP,
+
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, attempt to model it with some collection of primitives at the output. This
+ /// means that even if the incoming plan is completely optimal by some metric, the
+ /// returned plan is fairly likely to not be optimal anymore, and round-trips back and
+ /// forth may make the plan increasingly suboptimal. However, every primitive at the
+ /// output can be (manually) traced back to exactly one primitive at the input, which
+ /// may be useful when debugging.
+ PRESERVE_STRUCTURE,
+
+ /// Behaves like PRESERVE_STRUCTURE, but prefers performance over structural accuracy.
+ /// Basic optimizations *may* be applied, in order to attempt to not regress in terms of
+ /// plan performance: if the incoming plan was already aggressively optimized, the goal
+ /// is for the output plan to not be less performant. In practical use cases, this is
+ /// probably the option you want.
+ ///
+ /// Note that no guarantees are made on top of PRESERVE_STRUCTURE. Past and future
+ /// versions of Arrow may even ignore this option entirely and treat it exactly like
+ /// PRESERVE_STRUCTURE.
+ BEST_EFFORT,
+};
+
+/// Controls how to convert between Substrait and Acero representations of a plan.
Review Comment:
```suggestion
/// Options that control the conversion between Substrait and Acero representations of a plan.
```
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, reject the conversion. This effectively asserts that there is no (known)
+ /// information loss in the conversion, and that plans should either round-trip back and
+ /// forth exactly or not at all. This option is primarily intended for testing and
+ /// debugging.
+ EXACT_ROUNDTRIP,
+
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, attempt to model it with some collection of primitives at the output. This
+ /// means that even if the incoming plan is completely optimal by some metric, the
+ /// returned plan is fairly likely to not be optimal anymore, and round-trips back and
+ /// forth may make the plan increasingly suboptimal. However, every primitive at the
+ /// output can be (manually) traced back to exactly one primitive at the input, which
+ /// may be useful when debugging.
+ PRESERVE_STRUCTURE,
+
+ /// Behaves like PRESERVE_STRUCTURE, but prefers performance over structural accuracy.
+ /// Basic optimizations *may* be applied, in order to attempt to not regress in terms of
+ /// plan performance: if the incoming plan was already aggressively optimized, the goal
+ /// is for the output plan to not be less performant. In practical use cases, this is
+ /// probably the option you want.
+ ///
+ /// Note that no guarantees are made on top of PRESERVE_STRUCTURE. Past and future
+ /// versions of Arrow may even ignore this option entirely and treat it exactly like
+ /// PRESERVE_STRUCTURE.
+ BEST_EFFORT,
+};
+
+/// Controls how to convert between Substrait and Acero representations of a plan.
+struct ConversionOptions {
+ /// Controls how strictly the converter is to adhere to the structure of the input.
Review Comment:
```suggestion
/// \brief How strictly the converter should adhere to the structure of the input.
```
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, reject the conversion. This effectively asserts that there is no (known)
+ /// information loss in the conversion, and that plans should either round-trip back and
+ /// forth exactly or not at all. This option is primarily intended for testing and
+ /// debugging.
+ EXACT_ROUNDTRIP,
+
+ /// When a primitive is used at the input that doesn't have an exact match at the
+ /// output, attempt to model it with some collection of primitives at the output. This
+ /// means that even if the incoming plan is completely optimal by some metric, the
+ /// returned plan is fairly likely to not be optimal anymore, and round-trips back and
+ /// forth may make the plan increasingly suboptimal. However, every primitive at the
+ /// output can be (manually) traced back to exactly one primitive at the input, which
+ /// may be useful when debugging.
+ PRESERVE_STRUCTURE,
+
+ /// Behaves like PRESERVE_STRUCTURE, but prefers performance over structural accuracy.
+ /// Basic optimizations *may* be applied, in order to attempt to not regress in terms of
+ /// plan performance: if the incoming plan was already aggressively optimized, the goal
+ /// is for the output plan to not be less performant. In practical use cases, this is
+ /// probably the option you want.
+ ///
+ /// Note that no guarantees are made on top of PRESERVE_STRUCTURE. Past and future
+ /// versions of Arrow may even ignore this option entirely and treat it exactly like
+ /// PRESERVE_STRUCTURE.
+ BEST_EFFORT,
+};
+
+/// Controls how to convert between Substrait and Acero representations of a plan.
+struct ConversionOptions {
+ /// Controls how strictly the converter is to adhere to the structure of the input.
+ ConversionStrictness strictness = ConversionStrictness::EXACT_ROUNDTRIP;
Review Comment:
In the comments for `BEST_EFFORT` we say "In practical use cases, this is probably the option you want." so why isn't that the default?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1177342978
https://issues.apache.org/jira/browse/ARROW-16988
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on PR #13537:
URL: https://github.com/apache/arrow/pull/13537#issuecomment-1184915536
Rebased, s/PEDANTIC/EXACT_ROUNDTRIP/g, and reworded the comments.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jvanstraten commented on a diff in pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
jvanstraten commented on code in PR #13537:
URL: https://github.com/apache/arrow/pull/13537#discussion_r920425103
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#pragma once
+
+namespace arrow {
+namespace engine {
+
+/// How strictly to adhere to the input structure when converting between Substrait and
+/// Acero representations of a plan. This allows the user to trade conversion accuracy
+/// for performance and lenience.
+enum class ConversionStrictness {
+ /// Prevent information loss by rejecting incoming plans that use features or contain
+ /// metadata that cannot be exactly represented in the output format in a way that
+ /// will round-trip. Relations/nodes must map one-to-one.
+ PEDANTIC,
+
+ /// When an incoming plan uses a feature that cannot be exactly represented in the
+ /// output format, attempt to emulate that feature as opposed to immediately
+ /// rejecting the plan. For example, a Substrait SortRel with a complex sort key
+ /// expression may be emulated using a project-order-project triple. Relations/nodes
+ /// will thus map one-to-many.
+ PRESERVE_STRUCTURE,
+
+ /// Attempt to prevent performance-related regressions caused by differences in how
+ /// operations are represented in the input and output format, by allowing for
+ /// optimizations that cross structural boundaries. For example, the converter may
+ /// collapse chains of project nodes into one.
Review Comment:
> Isn't collapsing of a chain of project nodes more of an optimization than conversion concern? The collapsed node should be strictly equivalent to the original chain AFAIU.
My reasoning for doing this is stated much more clearly in the JIRA issue, but tl;dr, the problem is that Substrait and Acero differ to such an extent that you can't map one-to-one without loss of information in basically any practical use case. So, whenever a feature is used at the input that doesn't have an exact match at the output the option comes into play:
- PEDANTIC: reject the conversion; assert that there is no (known) information loss in the conversion. Plans should either round-trip back and forth exactly or not at all. Useful for testing and maybe debugging, but not really in practice. Note that this is the current behavior.
- PRESERVE_STRUCTURE: for every individual primitive in the incoming plan (node, expression, whatever) that can't be represented exactly in the output format, map to some collection of primitives instead to model the behavior, and only fail if this is also not possible. Even if the incoming plan is completely optimal, the returned plan probably won't be because of this expansion. Roundtrips back and forth will likely make the plan increasingly suboptimal. However, you could hypothetically add debug information to the plan a la gcc -O0 -g to trace every primitive back to exactly one primitive in the original plan.
- BEST_EFFORT: like PRESERVE_STRUCTURE, but prefer performance over structural accuracy; attempt to not regress in terms of plan performance. If the incoming plan was already aggressively optimized, the goal is for the output plan to not be substantially less performant.
My reasoning for making the latter part of the conversion is roughly:
- Some of these "optimizations" are very easy and performant to do while converting. Conversely, doing this afterwards requires building up an intermediate tree during conversion and then going over that tree again later. In fact, in some cases I've found it easier and less prone to dealing with special cases than exactly preserving structure.
- I foresee (hope for) a generic Substrait optimization engine. It'd be a shame if we can't leverage that, because the moment we convert from Substrait to Acero we immediately lose out on performance for trivial reasons.
That being said, if Acero gets its own optimizer at some point this option won't be as useful anymore. I also don't intend for BEST_EFFORT to do any fancy optimizations; in particular, anything that involves more than the tree traversal we already have to do for the conversion doesn't belong there. That is, indeed, the job of an optimization engine. As such, I've formulated the docs for the option such that it's perfectly permissible for BEST_EFFORT to have the same behavior as PRESERVE_STRUCTURE, in case we want to factor the logic into a smarter optimizer at some point.
> is BEST_EFFORT always more lenient than PRESERVE_STRUCTURE?
If by "lenient" you mean "how often it rejects a plan," with what I have in mind they'd be equal. However, I guess BEST_EFFORT could be more lenient, too. It certainly shouldn't be *less* lenient though.
> does BEST_EFFORT also allow the converse (relations/nodes mapping many-to-one)?
I'd say "many to one" is a subset of "many to many," so yes. The behavior I have in mind for it indeed also does that.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace merged pull request #13537: ARROW-16988: [C++] Introduce Substrait ToProto/FromProto conversion options
Posted by GitBox <gi...@apache.org>.
westonpace merged PR #13537:
URL: https://github.com/apache/arrow/pull/13537
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org