You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "westonpace (via GitHub)" <gi...@apache.org> on 2023/04/01 01:31:56 UTC
[GitHub] [arrow] westonpace opened a new pull request, #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
westonpace opened a new pull request, #34834:
URL: https://github.com/apache/arrow/pull/34834
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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] kou commented on a diff in pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1309447293
##########
python/pyarrow/tests/test_substrait.py:
##########
@@ -21,8 +21,10 @@
import pytest
import pyarrow as pa
+import pyarrow.compute as pc
+import pyarrow.dataset as ds
Review Comment:
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: 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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1669914825
I've rebased. Friendly ping @jorisvandenbossche if you have time to review this. If not, I plan on merging at the end of the week if things are still green.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1615998753
The appveyor failure seems valid (though utterly baffling): https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/47443264
--
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] jorisvandenbossche commented on a diff in pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1253079809
##########
python/pyarrow/_compute.pyx:
##########
@@ -2350,6 +2367,58 @@ cdef class Expression(_Weakrefable):
self.__class__.__name__, str(self)
)
+ @staticmethod
+ def from_substrait(object buffer not None):
+ """
+ Deserialize an expression from Substrait
+
+ The serialized message must be an ExtendedExpression message that has
+ only a single expression. The name of the expression and the schema
+ the expression was bound to will be ignored. Use
+ pyarrow.substrait.deserialize_expressions if this information is needed
+ or if the message might contain multiple expressions.
+
+ Parameters
+ ----------
+ buffer : bytes or Buffer
+ The Substrait message to deserialize
+
+ Returns
+ -------
+ Expression
+ The deserialized expression
+ """
+ expressions = _pas().deserialize_expressions(buffer).expressions
+ if len(expressions) == 0:
+ raise ValueError("Substrait message did not contain any expressions")
+ if len(expressions) > 1:
+ raise ValueError(
+ "Substrait message contained multiple expressions. Use pyarrow.substrait.deserialize_expressions instead")
+ return next(iter(expressions.values()))
+
+ def to_substrait(self, Schema schema not None, c_bool allow_udfs=False):
+ """
+ Serialize the expression using Substrait
+
+ The expression will be serialized as an ExtendedExpression message that has a
+ single expression named "expression"
+
+ Parameters
+ ----------
+ schema : Schema
+ The input schema the expression will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ A buffer containing the serialized Protobuf plan.
+ """
+ return _pas().serialize_expressions([self], "expression", schema, allow_udfs=allow_udfs)
Review Comment:
Allow the user to override this default name of "expression"? (although if you want that, you can always use `serialize_expressions` function instead)
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
Review Comment:
```suggestion
the Substrait expression ``a:i32 + b:i32`` is different from the
Substrait expression ``a:i64 + b:i64``. Pyarrow expressions are
```
? (that might be clearer that the actual field names are still "a" and "b" in both cases)
##########
python/pyarrow/_compute.pyx:
##########
@@ -2350,6 +2367,58 @@ cdef class Expression(_Weakrefable):
self.__class__.__name__, str(self)
)
+ @staticmethod
+ def from_substrait(object buffer not None):
+ """
+ Deserialize an expression from Substrait
+
+ The serialized message must be an ExtendedExpression message that has
+ only a single expression. The name of the expression and the schema
+ the expression was bound to will be ignored. Use
+ pyarrow.substrait.deserialize_expressions if this information is needed
+ or if the message might contain multiple expressions.
+
+ Parameters
+ ----------
+ buffer : bytes or Buffer
+ The Substrait message to deserialize
+
+ Returns
+ -------
+ Expression
+ The deserialized expression
+ """
+ expressions = _pas().deserialize_expressions(buffer).expressions
+ if len(expressions) == 0:
+ raise ValueError("Substrait message did not contain any expressions")
+ if len(expressions) > 1:
+ raise ValueError(
+ "Substrait message contained multiple expressions. Use pyarrow.substrait.deserialize_expressions instead")
+ return next(iter(expressions.values()))
+
+ def to_substrait(self, Schema schema not None, c_bool allow_udfs=False):
+ """
+ Serialize the expression using Substrait
+
+ The expression will be serialized as an ExtendedExpression message that has a
+ single expression named "expression"
+
+ Parameters
+ ----------
+ schema : Schema
+ The input schema the expression will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ A buffer containing the serialized Protobuf plan.
+ """
+ return _pas().serialize_expressions([self], "expression", schema, allow_udfs=allow_udfs)
Review Comment:
```suggestion
return _pas().serialize_expressions([self], ["expression"], schema, allow_udfs=allow_udfs)
```
This currently causes the bug that a deserialized form of this has "e" as name:
```
In [16]: expr = pc.field("a") == 1
In [17]: buf = expr.to_substrait(pa.schema([('a', 'int32')]))
In [18]: pyarrow.substrait.deserialize_expressions(buf).expressions
Out[18]: {'e': <pyarrow.compute.Expression (FieldPath(0) == 1)>}
```
(so might be good to add a test for this)
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
Review Comment:
The "udf" in the keyword name might be a bit confusing, as I think users of pyarrow will think in the form of actual UDFs defined by them, and not functions defined by arrow (but not part of substrait), as for the user, those are "built-in" functions, not UDFs.
I see that in the C++ code you are using `allow_arrow_extensions` as keyword. We can use that here as well? (or is there a specific reason you went for a different name?)
##########
python/pyarrow/_substrait.pyx:
##########
@@ -145,6 +146,7 @@ def run_query(plan, *, table_provider=None, use_threads=True):
}
c_conversion_options.named_table_provider = BindFunction[CNamedTableProvider](
&_create_named_table_provider, named_table_args)
+ c_conversion_options.allow_arrow_extensions = False
Review Comment:
This is the default, right? (i.e. this is not changing actual behaviour, just more explicit)
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ An ExtendedExpression message containing the serialized expressions
+ """
+ cdef:
+ CResult[shared_ptr[CBuffer]] c_res_buffer
+ shared_ptr[CBuffer] c_buffer
+ CNamedExpression c_named_expr
+ CBoundExpressions c_bound_exprs
+ CConversionOptions c_conversion_options
+
+ for i in range(len(exprs)):
+ if not isinstance(exprs[i], Expression):
+ raise TypeError(f"Expected Expression, got '{type(exprs[i])}'")
+ c_named_expr.expression = (<Expression> exprs[i]).unwrap()
+ if i < len(names):
+ if not isinstance(names[i], str):
+ raise TypeError(f"Expected str, got '{type(names[i])}'")
+ c_named_expr.name = tobytes(<str> names[i])
+ else:
+ c_named_expr.name = tobytes("autoname")
Review Comment:
Is this useful? (it can give multiple expressions with the same name?) Or could also raise an error instead?
I would maybe rather validate that `len(exprs) == len(names)`
(and in that case you can also do `for expr, name in zip(exprs, names): ..` to simplify the code a bit)
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ An ExtendedExpression message containing the serialized expressions
+ """
+ cdef:
+ CResult[shared_ptr[CBuffer]] c_res_buffer
+ shared_ptr[CBuffer] c_buffer
+ CNamedExpression c_named_expr
+ CBoundExpressions c_bound_exprs
+ CConversionOptions c_conversion_options
+
+ for i in range(len(exprs)):
+ if not isinstance(exprs[i], Expression):
+ raise TypeError(f"Expected Expression, got '{type(exprs[i])}'")
+ c_named_expr.expression = (<Expression> exprs[i]).unwrap()
+ if i < len(names):
+ if not isinstance(names[i], str):
+ raise TypeError(f"Expected str, got '{type(names[i])}'")
+ c_named_expr.name = tobytes(<str> names[i])
+ else:
+ c_named_expr.name = tobytes("autoname")
+ c_bound_exprs.named_expressions.push_back(c_named_expr)
+ c_bound_exprs.schema = (<Schema> schema).sp_schema
+
+ c_conversion_options.allow_arrow_extensions = allow_udfs
+
+ c_res_buffer = SerializeExpressions(c_bound_exprs, c_conversion_options)
+ with nogil:
+ c_buffer = GetResultValue(c_res_buffer)
Review Comment:
Can the `SerializeExpressions` be moved within the nogil block? (I assume this is the potentially costly part, the `GetResultValue` should always be cheap?)
##########
python/pyarrow/tests/test_substrait.py:
##########
@@ -923,3 +925,58 @@ def table_provider(names, _):
# Ordering of k is deterministic because this is running with serial execution
assert res_tb == expected_tb
+
+
+@pytest.mark.parametrize("expr", [
+ pc.equal(ds.field("x"), 7),
+ pc.equal(ds.field("x"), ds.field("y")),
+ ds.field("x") > 50
+])
+def test_serializing_expressions(expr):
+ schema = pa.schema([
+ pa.field("x", pa.int32()),
+ pa.field("y", pa.int32())
+ ])
+
+ buf = pa.substrait.serialize_expressions([expr], ["test_expr"], schema)
+ returned = pa.substrait.deserialize_expressions(buf)
+ assert schema == returned.schema
+ assert len(returned.expressions) == 1
+ assert "test_expr" in returned.expressions
+
+
+def test_serializing_multiple_expressions():
+ schema = pa.schema([
+ pa.field("x", pa.int32()),
+ pa.field("y", pa.int32())
+ ])
+ exprs = [pc.equal(ds.field("x"), 7), pc.equal(ds.field("x"), ds.field("y"))]
+ buf = pa.substrait.serialize_expressions(exprs, ["first", "second"], schema)
+ returned = pa.substrait.deserialize_expressions(buf)
+ assert schema == returned.schema
+ assert len(returned.expressions) == 2
+
+ norm_exprs = [pc.equal(ds.field(0), 7), pc.equal(ds.field(0), ds.field(1))]
+ assert str(returned.expressions["first"]) == str(norm_exprs[0])
+ assert str(returned.expressions["second"]) == str(norm_exprs[1])
Review Comment:
We also have an `equals` method on the Expression if you want to avoid string repr comparison (but not sure what the corner cases for either option)
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ An ExtendedExpression message containing the serialized expressions
+ """
+ cdef:
+ CResult[shared_ptr[CBuffer]] c_res_buffer
+ shared_ptr[CBuffer] c_buffer
+ CNamedExpression c_named_expr
+ CBoundExpressions c_bound_exprs
+ CConversionOptions c_conversion_options
+
+ for i in range(len(exprs)):
+ if not isinstance(exprs[i], Expression):
+ raise TypeError(f"Expected Expression, got '{type(exprs[i])}'")
+ c_named_expr.expression = (<Expression> exprs[i]).unwrap()
+ if i < len(names):
+ if not isinstance(names[i], str):
+ raise TypeError(f"Expected str, got '{type(names[i])}'")
+ c_named_expr.name = tobytes(<str> names[i])
+ else:
+ c_named_expr.name = tobytes("autoname")
+ c_bound_exprs.named_expressions.push_back(c_named_expr)
+ c_bound_exprs.schema = (<Schema> schema).sp_schema
+
+ c_conversion_options.allow_arrow_extensions = allow_udfs
+
+ c_res_buffer = SerializeExpressions(c_bound_exprs, c_conversion_options)
+ with nogil:
+ c_buffer = GetResultValue(c_res_buffer)
+ return pyarrow_wrap_buffer(c_buffer)
+
+
+cdef class BoundExpressions(_Weakrefable):
+ """
+ A collection of named expressions and the schema they are bound to
+
+ This is equivalent to the Substrait ExtendedExpression message
+ """
+
+ cdef:
+ CBoundExpressions c_bound_exprs
+
+ def __init__(self):
+ msg = 'BoundExpressions is an abstract class thus cannot be initialized.'
+ raise TypeError(msg)
+
+ cdef void init(self, CBoundExpressions bound_expressions):
+ self.c_bound_exprs = bound_expressions
+
+ @property
+ def schema(self):
+ """
+ The common schema that all expressions are bound to
+ """
+ return pyarrow_wrap_schema(self.c_bound_exprs.schema)
+
+ @property
+ def expressions(self):
+ """
+ A dict from expression name to expression
+ """
+ expr_dict = {}
+ for named_expr in self.c_bound_exprs.named_expressions:
+ name = frombytes(named_expr.name)
+ expr = Expression.wrap(named_expr.expression)
+ expr_dict[name] = expr
+ return expr_dict
+
+ @staticmethod
+ cdef wrap(const CBoundExpressions& bound_expressions):
+ cdef BoundExpressions self = BoundExpressions.__new__(BoundExpressions)
+ self.init(bound_expressions)
+ return self
+
+
+def deserialize_expressions(buf):
+ """
+ Deserialize an ExtendedExpression Substrait message into a BoundExpressions object
+
+ Parameters
+ ----------
+ buf : Buffer or bytes
+ The message to deserialize
+
+ Returns
+ -------
+ BoundExpressions
+ The deserialized expressions, their names, and the bound schema
+ """
+ cdef:
+ shared_ptr[CBuffer] c_buffer
+ CResult[CBoundExpressions] c_res_bound_exprs
+ CBoundExpressions c_bound_exprs
+
+ if isinstance(buf, bytes):
+ c_buffer = pyarrow_unwrap_buffer(py_buffer(buf))
+ elif isinstance(buf, Buffer):
+ c_buffer = pyarrow_unwrap_buffer(buf)
+ else:
+ raise TypeError(
+ f"Expected 'pyarrow.Buffer' or bytes, got '{type(buf)}'")
+
+ c_res_bound_exprs = DeserializeExpressions(deref(c_buffer))
+ with nogil:
+ c_bound_exprs = GetResultValue(c_res_bound_exprs)
Review Comment:
Same question here about nogil
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -3277,19 +3281,106 @@ def test_expression_serialization():
f = pc.scalar({'a': 1})
g = pc.scalar(pa.scalar(1))
h = pc.scalar(np.int64(2))
+ j = pc.scalar(False)
+
+ literal_exprs = [a, b, c, d, e, g, h, j]
+
+ exprs_with_call = [a == b, a != b, a > b, c & j, c | j, ~c, d.is_valid(),
+ a + b, a - b, a * b, a / b, pc.negate(a),
+ pc.add(a, b), pc.subtract(a, b), pc.divide(a, b),
+ pc.multiply(a, b), pc.power(a, a), pc.sqrt(a),
+ pc.exp(b), pc.cos(b), pc.sin(b), pc.tan(b),
+ pc.acos(b), pc.atan(b), pc.asin(b), pc.atan2(b, b),
+ pc.abs(b), pc.sign(a), pc.bit_wise_not(a),
+ pc.bit_wise_and(a, a), pc.bit_wise_or(a, a),
+ pc.bit_wise_xor(a, a), pc.is_nan(b), pc.is_finite(b),
+ pc.coalesce(a, b),
+ a.cast(pa.int32(), safe=False)]
+
+ exprs_with_ref = [pc.field('i64') > 5, pc.field('i64') == 5,
+ pc.field('i64') == 7,
+ pc.field(('foo', 'bar')) == 'value',
+ pc.field('foo', 'bar') == 'value']
+
+ special_cases = [
+ f, # Struct literals lose their field names
+ a.isin([1, 2, 3]), # isin converts to an or list
+ pc.field('i64').is_null() # pyarrow always specifies a FunctionOptions
+ # for is_null which, being the default, is
+ # dropped on serialization
+ ]
- all_exprs = [a, b, c, d, e, f, g, h, a == b, a > b, a & b, a | b, ~c,
- d.is_valid(), a.cast(pa.int32(), safe=False),
- a.cast(pa.int32(), safe=False), a.isin([1, 2, 3]),
- pc.field('i64') > 5, pc.field('i64') == 5,
- pc.field('i64') == 7, pc.field('i64').is_null(),
- pc.field(('foo', 'bar')) == 'value',
- pc.field('foo', 'bar') == 'value']
+ all_exprs = literal_exprs + exprs_with_call + exprs_with_ref + special_cases
for expr in all_exprs:
assert isinstance(expr, pc.Expression)
restored = pickle.loads(pickle.dumps(expr))
assert expr.equals(restored)
+ if pas is not None:
+
+ test_schema = pa.schema([pa.field("i64", pa.int64()), pa.field(
+ "foo", pa.struct([pa.field("bar", pa.string())]))])
+
+ # Basic literals don't change on binding and so they will round
+ # trip without any change
+ for expr in literal_exprs:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert expr.equals(deserialized)
+
+ # Expressions are bound when they get serialized. Since bound
+ # expressions are not equal to their unbound variants we cannot
+ # compare the round tripped with the original
+ for expr in exprs_with_call:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ # We can't compare the expressions themselves because of the bound
+ # unbound difference. But we can compare the string representation
+ assert str(deserialized) == str(expr)
+ serialized_again = deserialized.to_substrait(test_schema)
+ deserialized_again = pc.Expression.from_substrait(serialized_again)
+ assert deserialized.equals(deserialized_again)
+
+ # Expressions that have references will be normalized, on serialization,
+ # to numeric referneces
Review Comment:
```suggestion
# to numeric references
```
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -3277,19 +3281,106 @@ def test_expression_serialization():
f = pc.scalar({'a': 1})
g = pc.scalar(pa.scalar(1))
h = pc.scalar(np.int64(2))
+ j = pc.scalar(False)
+
+ literal_exprs = [a, b, c, d, e, g, h, j]
+
+ exprs_with_call = [a == b, a != b, a > b, c & j, c | j, ~c, d.is_valid(),
+ a + b, a - b, a * b, a / b, pc.negate(a),
+ pc.add(a, b), pc.subtract(a, b), pc.divide(a, b),
+ pc.multiply(a, b), pc.power(a, a), pc.sqrt(a),
+ pc.exp(b), pc.cos(b), pc.sin(b), pc.tan(b),
+ pc.acos(b), pc.atan(b), pc.asin(b), pc.atan2(b, b),
+ pc.abs(b), pc.sign(a), pc.bit_wise_not(a),
+ pc.bit_wise_and(a, a), pc.bit_wise_or(a, a),
+ pc.bit_wise_xor(a, a), pc.is_nan(b), pc.is_finite(b),
+ pc.coalesce(a, b),
+ a.cast(pa.int32(), safe=False)]
+
+ exprs_with_ref = [pc.field('i64') > 5, pc.field('i64') == 5,
+ pc.field('i64') == 7,
+ pc.field(('foo', 'bar')) == 'value',
+ pc.field('foo', 'bar') == 'value']
+
+ special_cases = [
+ f, # Struct literals lose their field names
+ a.isin([1, 2, 3]), # isin converts to an or list
+ pc.field('i64').is_null() # pyarrow always specifies a FunctionOptions
+ # for is_null which, being the default, is
+ # dropped on serialization
+ ]
- all_exprs = [a, b, c, d, e, f, g, h, a == b, a > b, a & b, a | b, ~c,
- d.is_valid(), a.cast(pa.int32(), safe=False),
- a.cast(pa.int32(), safe=False), a.isin([1, 2, 3]),
- pc.field('i64') > 5, pc.field('i64') == 5,
- pc.field('i64') == 7, pc.field('i64').is_null(),
- pc.field(('foo', 'bar')) == 'value',
- pc.field('foo', 'bar') == 'value']
+ all_exprs = literal_exprs + exprs_with_call + exprs_with_ref + special_cases
for expr in all_exprs:
assert isinstance(expr, pc.Expression)
restored = pickle.loads(pickle.dumps(expr))
assert expr.equals(restored)
+ if pas is not None:
+
+ test_schema = pa.schema([pa.field("i64", pa.int64()), pa.field(
+ "foo", pa.struct([pa.field("bar", pa.string())]))])
+
+ # Basic literals don't change on binding and so they will round
+ # trip without any change
+ for expr in literal_exprs:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert expr.equals(deserialized)
+
+ # Expressions are bound when they get serialized. Since bound
+ # expressions are not equal to their unbound variants we cannot
+ # compare the round tripped with the original
+ for expr in exprs_with_call:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ # We can't compare the expressions themselves because of the bound
+ # unbound difference. But we can compare the string representation
Review Comment:
This one I don't fully understand: this are expressions with calls but without any field reference, only with scalars which already have a type. So why is bound/unbound relevant in this case? (I would have expected that only be relevant for references to fields in the schema)
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -3277,19 +3281,106 @@ def test_expression_serialization():
f = pc.scalar({'a': 1})
g = pc.scalar(pa.scalar(1))
h = pc.scalar(np.int64(2))
+ j = pc.scalar(False)
+
+ literal_exprs = [a, b, c, d, e, g, h, j]
+
+ exprs_with_call = [a == b, a != b, a > b, c & j, c | j, ~c, d.is_valid(),
+ a + b, a - b, a * b, a / b, pc.negate(a),
+ pc.add(a, b), pc.subtract(a, b), pc.divide(a, b),
+ pc.multiply(a, b), pc.power(a, a), pc.sqrt(a),
+ pc.exp(b), pc.cos(b), pc.sin(b), pc.tan(b),
+ pc.acos(b), pc.atan(b), pc.asin(b), pc.atan2(b, b),
+ pc.abs(b), pc.sign(a), pc.bit_wise_not(a),
+ pc.bit_wise_and(a, a), pc.bit_wise_or(a, a),
+ pc.bit_wise_xor(a, a), pc.is_nan(b), pc.is_finite(b),
+ pc.coalesce(a, b),
+ a.cast(pa.int32(), safe=False)]
+
+ exprs_with_ref = [pc.field('i64') > 5, pc.field('i64') == 5,
+ pc.field('i64') == 7,
+ pc.field(('foo', 'bar')) == 'value',
+ pc.field('foo', 'bar') == 'value']
+
+ special_cases = [
+ f, # Struct literals lose their field names
+ a.isin([1, 2, 3]), # isin converts to an or list
+ pc.field('i64').is_null() # pyarrow always specifies a FunctionOptions
+ # for is_null which, being the default, is
+ # dropped on serialization
+ ]
- all_exprs = [a, b, c, d, e, f, g, h, a == b, a > b, a & b, a | b, ~c,
- d.is_valid(), a.cast(pa.int32(), safe=False),
- a.cast(pa.int32(), safe=False), a.isin([1, 2, 3]),
- pc.field('i64') > 5, pc.field('i64') == 5,
- pc.field('i64') == 7, pc.field('i64').is_null(),
- pc.field(('foo', 'bar')) == 'value',
- pc.field('foo', 'bar') == 'value']
+ all_exprs = literal_exprs + exprs_with_call + exprs_with_ref + special_cases
for expr in all_exprs:
assert isinstance(expr, pc.Expression)
restored = pickle.loads(pickle.dumps(expr))
assert expr.equals(restored)
+ if pas is not None:
Review Comment:
It _might_ be nicer to split out the substrait-based serialization to a separate test, but then we need to factor out the expression creation into a helper function? (it's certainly OK to just leave as is)
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -3277,19 +3281,106 @@ def test_expression_serialization():
f = pc.scalar({'a': 1})
g = pc.scalar(pa.scalar(1))
h = pc.scalar(np.int64(2))
+ j = pc.scalar(False)
+
+ literal_exprs = [a, b, c, d, e, g, h, j]
+
+ exprs_with_call = [a == b, a != b, a > b, c & j, c | j, ~c, d.is_valid(),
+ a + b, a - b, a * b, a / b, pc.negate(a),
+ pc.add(a, b), pc.subtract(a, b), pc.divide(a, b),
+ pc.multiply(a, b), pc.power(a, a), pc.sqrt(a),
+ pc.exp(b), pc.cos(b), pc.sin(b), pc.tan(b),
+ pc.acos(b), pc.atan(b), pc.asin(b), pc.atan2(b, b),
+ pc.abs(b), pc.sign(a), pc.bit_wise_not(a),
+ pc.bit_wise_and(a, a), pc.bit_wise_or(a, a),
+ pc.bit_wise_xor(a, a), pc.is_nan(b), pc.is_finite(b),
+ pc.coalesce(a, b),
+ a.cast(pa.int32(), safe=False)]
+
+ exprs_with_ref = [pc.field('i64') > 5, pc.field('i64') == 5,
+ pc.field('i64') == 7,
+ pc.field(('foo', 'bar')) == 'value',
+ pc.field('foo', 'bar') == 'value']
+
+ special_cases = [
+ f, # Struct literals lose their field names
+ a.isin([1, 2, 3]), # isin converts to an or list
+ pc.field('i64').is_null() # pyarrow always specifies a FunctionOptions
+ # for is_null which, being the default, is
+ # dropped on serialization
+ ]
- all_exprs = [a, b, c, d, e, f, g, h, a == b, a > b, a & b, a | b, ~c,
- d.is_valid(), a.cast(pa.int32(), safe=False),
- a.cast(pa.int32(), safe=False), a.isin([1, 2, 3]),
- pc.field('i64') > 5, pc.field('i64') == 5,
- pc.field('i64') == 7, pc.field('i64').is_null(),
- pc.field(('foo', 'bar')) == 'value',
- pc.field('foo', 'bar') == 'value']
+ all_exprs = literal_exprs + exprs_with_call + exprs_with_ref + special_cases
for expr in all_exprs:
assert isinstance(expr, pc.Expression)
restored = pickle.loads(pickle.dumps(expr))
assert expr.equals(restored)
+ if pas is not None:
+
+ test_schema = pa.schema([pa.field("i64", pa.int64()), pa.field(
+ "foo", pa.struct([pa.field("bar", pa.string())]))])
+
+ # Basic literals don't change on binding and so they will round
+ # trip without any change
+ for expr in literal_exprs:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert expr.equals(deserialized)
+
+ # Expressions are bound when they get serialized. Since bound
+ # expressions are not equal to their unbound variants we cannot
+ # compare the round tripped with the original
+ for expr in exprs_with_call:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ # We can't compare the expressions themselves because of the bound
+ # unbound difference. But we can compare the string representation
+ assert str(deserialized) == str(expr)
+ serialized_again = deserialized.to_substrait(test_schema)
+ deserialized_again = pc.Expression.from_substrait(serialized_again)
+ assert deserialized.equals(deserialized_again)
+
+ # Expressions that have references will be normalized, on serialization,
+ # to numeric referneces
+ exprs_with_ref_norm = [pc.field(0) > 5, pc.field(0) == 5,
+ pc.field(0) == 7,
+ pc.field((1, 0)) == 'value',
+ pc.field(1, 0) == 'value']
+
+ for expr, expr_norm in zip(exprs_with_ref, exprs_with_ref_norm):
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert str(deserialized) == str(expr_norm)
+ serialized_again = deserialized.to_substrait(test_schema)
+ deserialized_again = pc.Expression.from_substrait(serialized_again)
+ assert deserialized.equals(deserialized_again)
+
+ # For the special cases we get various wrinkles in serialization but we
+ # should always get the same thing from round tripping twice
+ for expr in special_cases:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ serialized_again = deserialized.to_substrait(test_schema)
+ deserialized_again = pc.Expression.from_substrait(serialized_again)
+ assert deserialized.equals(deserialized_again)
+
+ # Special case, we lose the field names of struct literals
+ serialized = f.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert deserialized.equals(pc.scalar({'': 1}))
Review Comment:
Should we use something else than an empty string? (eg "field0", "field1", ..)
(although this is probably enough of a corner case to not put too much effort in 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] westonpace commented on a diff in pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1256554553
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ An ExtendedExpression message containing the serialized expressions
+ """
+ cdef:
+ CResult[shared_ptr[CBuffer]] c_res_buffer
+ shared_ptr[CBuffer] c_buffer
+ CNamedExpression c_named_expr
+ CBoundExpressions c_bound_exprs
+ CConversionOptions c_conversion_options
+
+ for i in range(len(exprs)):
+ if not isinstance(exprs[i], Expression):
+ raise TypeError(f"Expected Expression, got '{type(exprs[i])}'")
+ c_named_expr.expression = (<Expression> exprs[i]).unwrap()
+ if i < len(names):
+ if not isinstance(names[i], str):
+ raise TypeError(f"Expected str, got '{type(names[i])}'")
+ c_named_expr.name = tobytes(<str> names[i])
+ else:
+ c_named_expr.name = tobytes("autoname")
Review Comment:
I've updated this to now raise an error if the length of names and exprs doesn't match (and I use zip now and added a test case).
--
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] kou commented on a diff in pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1306365538
##########
python/pyarrow/tests/test_substrait.py:
##########
@@ -21,8 +21,10 @@
import pytest
import pyarrow as pa
+import pyarrow.compute as pc
+import pyarrow.dataset as ds
Review Comment:
@westonpace It seems that we can't import `pyarrow.dataset` unconditionally: https://github.com/ursacomputing/crossbow/actions/runs/5957902192/job/16161350103#step:3:8002
```text
==================================== ERRORS ====================================
_______________ ERROR collecting pyarrow/tests/test_substrait.py _______________
ImportError while importing test module '/arrow/python/pyarrow/tests/test_substrait.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/miniconda-for-arrow/envs/pyarrow-3.10/lib/python3.10/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
pyarrow/tests/test_substrait.py:25: in <module>
import pyarrow.dataset as ds
pyarrow/dataset.py:58: in <module>
raise ImportError(
E ImportError: The pyarrow installation is not built with support for 'dataset' (No module named 'pyarrow._dataset')
```
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1255233632
##########
python/pyarrow/_substrait.pyx:
##########
@@ -145,6 +146,7 @@ def run_query(plan, *, table_provider=None, use_threads=True):
}
c_conversion_options.named_table_provider = BindFunction[CNamedTableProvider](
&_create_named_table_provider, named_table_args)
+ c_conversion_options.allow_arrow_extensions = False
Review Comment:
Yes, this is the default. I can leave it off.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1256555771
##########
python/pyarrow/tests/test_substrait.py:
##########
@@ -923,3 +925,58 @@ def table_provider(names, _):
# Ordering of k is deterministic because this is running with serial execution
assert res_tb == expected_tb
+
+
+@pytest.mark.parametrize("expr", [
+ pc.equal(ds.field("x"), 7),
+ pc.equal(ds.field("x"), ds.field("y")),
+ ds.field("x") > 50
+])
+def test_serializing_expressions(expr):
+ schema = pa.schema([
+ pa.field("x", pa.int32()),
+ pa.field("y", pa.int32())
+ ])
+
+ buf = pa.substrait.serialize_expressions([expr], ["test_expr"], schema)
+ returned = pa.substrait.deserialize_expressions(buf)
+ assert schema == returned.schema
+ assert len(returned.expressions) == 1
+ assert "test_expr" in returned.expressions
+
+
+def test_serializing_multiple_expressions():
+ schema = pa.schema([
+ pa.field("x", pa.int32()),
+ pa.field("y", pa.int32())
+ ])
+ exprs = [pc.equal(ds.field("x"), 7), pc.equal(ds.field("x"), ds.field("y"))]
+ buf = pa.substrait.serialize_expressions(exprs, ["first", "second"], schema)
+ returned = pa.substrait.deserialize_expressions(buf)
+ assert schema == returned.schema
+ assert len(returned.expressions) == 2
+
+ norm_exprs = [pc.equal(ds.field(0), 7), pc.equal(ds.field(0), ds.field(1))]
+ assert str(returned.expressions["first"]) == str(norm_exprs[0])
+ assert str(returned.expressions["second"]) == str(norm_exprs[1])
Review Comment:
As mentioned below this doesn't work because of the kernels binding.
--
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] conbench-apache-arrow[bot] commented on pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1689414722
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 702e9cae138e8d372643e3545375d8fc4c6212a2.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/16131599686) has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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] ianmcook commented on a diff in pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1309348122
##########
python/pyarrow/tests/test_substrait.py:
##########
@@ -21,8 +21,10 @@
import pytest
import pyarrow as pa
+import pyarrow.compute as pc
+import pyarrow.dataset as ds
Review Comment:
I opened https://github.com/apache/arrow/issues/37449 for this.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1492777652
* Closes: #33985
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1256555949
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -3277,19 +3281,106 @@ def test_expression_serialization():
f = pc.scalar({'a': 1})
g = pc.scalar(pa.scalar(1))
h = pc.scalar(np.int64(2))
+ j = pc.scalar(False)
+
+ literal_exprs = [a, b, c, d, e, g, h, j]
+
+ exprs_with_call = [a == b, a != b, a > b, c & j, c | j, ~c, d.is_valid(),
+ a + b, a - b, a * b, a / b, pc.negate(a),
+ pc.add(a, b), pc.subtract(a, b), pc.divide(a, b),
+ pc.multiply(a, b), pc.power(a, a), pc.sqrt(a),
+ pc.exp(b), pc.cos(b), pc.sin(b), pc.tan(b),
+ pc.acos(b), pc.atan(b), pc.asin(b), pc.atan2(b, b),
+ pc.abs(b), pc.sign(a), pc.bit_wise_not(a),
+ pc.bit_wise_and(a, a), pc.bit_wise_or(a, a),
+ pc.bit_wise_xor(a, a), pc.is_nan(b), pc.is_finite(b),
+ pc.coalesce(a, b),
+ a.cast(pa.int32(), safe=False)]
+
+ exprs_with_ref = [pc.field('i64') > 5, pc.field('i64') == 5,
+ pc.field('i64') == 7,
+ pc.field(('foo', 'bar')) == 'value',
+ pc.field('foo', 'bar') == 'value']
+
+ special_cases = [
+ f, # Struct literals lose their field names
+ a.isin([1, 2, 3]), # isin converts to an or list
+ pc.field('i64').is_null() # pyarrow always specifies a FunctionOptions
+ # for is_null which, being the default, is
+ # dropped on serialization
+ ]
- all_exprs = [a, b, c, d, e, f, g, h, a == b, a > b, a & b, a | b, ~c,
- d.is_valid(), a.cast(pa.int32(), safe=False),
- a.cast(pa.int32(), safe=False), a.isin([1, 2, 3]),
- pc.field('i64') > 5, pc.field('i64') == 5,
- pc.field('i64') == 7, pc.field('i64').is_null(),
- pc.field(('foo', 'bar')) == 'value',
- pc.field('foo', 'bar') == 'value']
+ all_exprs = literal_exprs + exprs_with_call + exprs_with_ref + special_cases
for expr in all_exprs:
assert isinstance(expr, pc.Expression)
restored = pickle.loads(pickle.dumps(expr))
assert expr.equals(restored)
+ if pas is not None:
Review Comment:
I went ahead and split them out.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1256555421
##########
python/pyarrow/_substrait.pyx:
##########
@@ -145,6 +146,7 @@ def run_query(plan, *, table_provider=None, use_threads=True):
}
c_conversion_options.named_table_provider = BindFunction[CNamedTableProvider](
&_create_named_table_provider, named_table_args)
+ c_conversion_options.allow_arrow_extensions = False
Review Comment:
I've removed this statement.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1629376038
I've fixed the appveyor issue. The remaining CI failures seem to be unrelated. @jorisvandenbossche I don't know if you'll be able to give this another review? I would like to get this into the release so people can start experimenting with 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] westonpace commented on a diff in pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1255238920
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -3277,19 +3281,106 @@ def test_expression_serialization():
f = pc.scalar({'a': 1})
g = pc.scalar(pa.scalar(1))
h = pc.scalar(np.int64(2))
+ j = pc.scalar(False)
+
+ literal_exprs = [a, b, c, d, e, g, h, j]
+
+ exprs_with_call = [a == b, a != b, a > b, c & j, c | j, ~c, d.is_valid(),
+ a + b, a - b, a * b, a / b, pc.negate(a),
+ pc.add(a, b), pc.subtract(a, b), pc.divide(a, b),
+ pc.multiply(a, b), pc.power(a, a), pc.sqrt(a),
+ pc.exp(b), pc.cos(b), pc.sin(b), pc.tan(b),
+ pc.acos(b), pc.atan(b), pc.asin(b), pc.atan2(b, b),
+ pc.abs(b), pc.sign(a), pc.bit_wise_not(a),
+ pc.bit_wise_and(a, a), pc.bit_wise_or(a, a),
+ pc.bit_wise_xor(a, a), pc.is_nan(b), pc.is_finite(b),
+ pc.coalesce(a, b),
+ a.cast(pa.int32(), safe=False)]
+
+ exprs_with_ref = [pc.field('i64') > 5, pc.field('i64') == 5,
+ pc.field('i64') == 7,
+ pc.field(('foo', 'bar')) == 'value',
+ pc.field('foo', 'bar') == 'value']
+
+ special_cases = [
+ f, # Struct literals lose their field names
+ a.isin([1, 2, 3]), # isin converts to an or list
+ pc.field('i64').is_null() # pyarrow always specifies a FunctionOptions
+ # for is_null which, being the default, is
+ # dropped on serialization
+ ]
- all_exprs = [a, b, c, d, e, f, g, h, a == b, a > b, a & b, a | b, ~c,
- d.is_valid(), a.cast(pa.int32(), safe=False),
- a.cast(pa.int32(), safe=False), a.isin([1, 2, 3]),
- pc.field('i64') > 5, pc.field('i64') == 5,
- pc.field('i64') == 7, pc.field('i64').is_null(),
- pc.field(('foo', 'bar')) == 'value',
- pc.field('foo', 'bar') == 'value']
+ all_exprs = literal_exprs + exprs_with_call + exprs_with_ref + special_cases
for expr in all_exprs:
assert isinstance(expr, pc.Expression)
restored = pickle.loads(pickle.dumps(expr))
assert expr.equals(restored)
+ if pas is not None:
+
+ test_schema = pa.schema([pa.field("i64", pa.int64()), pa.field(
+ "foo", pa.struct([pa.field("bar", pa.string())]))])
+
+ # Basic literals don't change on binding and so they will round
+ # trip without any change
+ for expr in literal_exprs:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert expr.equals(deserialized)
+
+ # Expressions are bound when they get serialized. Since bound
+ # expressions are not equal to their unbound variants we cannot
+ # compare the round tripped with the original
+ for expr in exprs_with_call:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ # We can't compare the expressions themselves because of the bound
+ # unbound difference. But we can compare the string representation
+ assert str(deserialized) == str(expr)
+ serialized_again = deserialized.to_substrait(test_schema)
+ deserialized_again = pc.Expression.from_substrait(serialized_again)
+ assert deserialized.equals(deserialized_again)
+
+ # Expressions that have references will be normalized, on serialization,
+ # to numeric referneces
+ exprs_with_ref_norm = [pc.field(0) > 5, pc.field(0) == 5,
+ pc.field(0) == 7,
+ pc.field((1, 0)) == 'value',
+ pc.field(1, 0) == 'value']
+
+ for expr, expr_norm in zip(exprs_with_ref, exprs_with_ref_norm):
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert str(deserialized) == str(expr_norm)
+ serialized_again = deserialized.to_substrait(test_schema)
+ deserialized_again = pc.Expression.from_substrait(serialized_again)
+ assert deserialized.equals(deserialized_again)
+
+ # For the special cases we get various wrinkles in serialization but we
+ # should always get the same thing from round tripping twice
+ for expr in special_cases:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ serialized_again = deserialized.to_substrait(test_schema)
+ deserialized_again = pc.Expression.from_substrait(serialized_again)
+ assert deserialized.equals(deserialized_again)
+
+ # Special case, we lose the field names of struct literals
+ serialized = f.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert deserialized.equals(pc.scalar({'': 1}))
Review Comment:
Hmm, I think I'd rather leave it this way for now so it's more obvious that the fields were lost and not that they got mixed up with some other schema. This is a bit of an obnoxious corner case. We might want to fix this on the Substrait side.
--
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] jorisvandenbossche commented on a diff in pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1256013840
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -3277,19 +3281,106 @@ def test_expression_serialization():
f = pc.scalar({'a': 1})
g = pc.scalar(pa.scalar(1))
h = pc.scalar(np.int64(2))
+ j = pc.scalar(False)
+
+ literal_exprs = [a, b, c, d, e, g, h, j]
+
+ exprs_with_call = [a == b, a != b, a > b, c & j, c | j, ~c, d.is_valid(),
+ a + b, a - b, a * b, a / b, pc.negate(a),
+ pc.add(a, b), pc.subtract(a, b), pc.divide(a, b),
+ pc.multiply(a, b), pc.power(a, a), pc.sqrt(a),
+ pc.exp(b), pc.cos(b), pc.sin(b), pc.tan(b),
+ pc.acos(b), pc.atan(b), pc.asin(b), pc.atan2(b, b),
+ pc.abs(b), pc.sign(a), pc.bit_wise_not(a),
+ pc.bit_wise_and(a, a), pc.bit_wise_or(a, a),
+ pc.bit_wise_xor(a, a), pc.is_nan(b), pc.is_finite(b),
+ pc.coalesce(a, b),
+ a.cast(pa.int32(), safe=False)]
+
+ exprs_with_ref = [pc.field('i64') > 5, pc.field('i64') == 5,
+ pc.field('i64') == 7,
+ pc.field(('foo', 'bar')) == 'value',
+ pc.field('foo', 'bar') == 'value']
+
+ special_cases = [
+ f, # Struct literals lose their field names
+ a.isin([1, 2, 3]), # isin converts to an or list
+ pc.field('i64').is_null() # pyarrow always specifies a FunctionOptions
+ # for is_null which, being the default, is
+ # dropped on serialization
+ ]
- all_exprs = [a, b, c, d, e, f, g, h, a == b, a > b, a & b, a | b, ~c,
- d.is_valid(), a.cast(pa.int32(), safe=False),
- a.cast(pa.int32(), safe=False), a.isin([1, 2, 3]),
- pc.field('i64') > 5, pc.field('i64') == 5,
- pc.field('i64') == 7, pc.field('i64').is_null(),
- pc.field(('foo', 'bar')) == 'value',
- pc.field('foo', 'bar') == 'value']
+ all_exprs = literal_exprs + exprs_with_call + exprs_with_ref + special_cases
for expr in all_exprs:
assert isinstance(expr, pc.Expression)
restored = pickle.loads(pickle.dumps(expr))
assert expr.equals(restored)
+ if pas is not None:
+
+ test_schema = pa.schema([pa.field("i64", pa.int64()), pa.field(
+ "foo", pa.struct([pa.field("bar", pa.string())]))])
+
+ # Basic literals don't change on binding and so they will round
+ # trip without any change
+ for expr in literal_exprs:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert expr.equals(deserialized)
+
+ # Expressions are bound when they get serialized. Since bound
+ # expressions are not equal to their unbound variants we cannot
+ # compare the round tripped with the original
+ for expr in exprs_with_call:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ # We can't compare the expressions themselves because of the bound
+ # unbound difference. But we can compare the string representation
Review Comment:
I see, thanks for the explanation! I hadn't considered that the function vs kernel distinction
--
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] ianmcook commented on pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1731716818
In case this helps anyone, there's an example here showing how this can be used through Python: https://gist.github.com/ianmcook/f70fc185d29ae97bdf85ffe0378c68e0
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1626269773
Hmmm...seems I've botched git. Apologies. Will fix.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1255233439
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ An ExtendedExpression message containing the serialized expressions
+ """
+ cdef:
+ CResult[shared_ptr[CBuffer]] c_res_buffer
+ shared_ptr[CBuffer] c_buffer
+ CNamedExpression c_named_expr
+ CBoundExpressions c_bound_exprs
+ CConversionOptions c_conversion_options
+
+ for i in range(len(exprs)):
+ if not isinstance(exprs[i], Expression):
+ raise TypeError(f"Expected Expression, got '{type(exprs[i])}'")
+ c_named_expr.expression = (<Expression> exprs[i]).unwrap()
+ if i < len(names):
+ if not isinstance(names[i], str):
+ raise TypeError(f"Expected str, got '{type(names[i])}'")
+ c_named_expr.name = tobytes(<str> names[i])
+ else:
+ c_named_expr.name = tobytes("autoname")
+ c_bound_exprs.named_expressions.push_back(c_named_expr)
+ c_bound_exprs.schema = (<Schema> schema).sp_schema
+
+ c_conversion_options.allow_arrow_extensions = allow_udfs
+
+ c_res_buffer = SerializeExpressions(c_bound_exprs, c_conversion_options)
+ with nogil:
+ c_buffer = GetResultValue(c_res_buffer)
Review Comment:
Probably just copy/paste :). Yes, I will update this.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1624731983
> And substrait doesn't have an "is_in" like function? (or are there plans for that?)
> (this conversion seems unfortunate, as "is_in" can be more efficient than the equivalent or-list)
It's an interesting point. We have things like this outside of expressions too. For example, the "join" node doesn't distinguish between an equality join (which can be done efficiently with a hashmap) and a non-equality join (which cannot). In that case we actually have both representations. The one people typically use is the "JoinRel" which is a logical operator and thus allowed to be more generic without concern for efficiency and the other one is the "HashJoinRel" which is more specific / physical, but typically not created by producers (instead planners or optimizers convert from one to the other).
I think this is interesting because "is_in" vs. "singular-or-list" is basically a logical vs physical distinction for expressions which I don't think I've really considered before, but I agree with you its valid.
In any case, it will be easy enough in Acero's converter, to recognize the cases that can collapse to `is_in` and use it where appropriate. I've created https://github.com/apache/arrow/issues/36535 to track this.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1256555582
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ An ExtendedExpression message containing the serialized expressions
+ """
+ cdef:
+ CResult[shared_ptr[CBuffer]] c_res_buffer
+ shared_ptr[CBuffer] c_buffer
+ CNamedExpression c_named_expr
+ CBoundExpressions c_bound_exprs
+ CConversionOptions c_conversion_options
+
+ for i in range(len(exprs)):
+ if not isinstance(exprs[i], Expression):
+ raise TypeError(f"Expected Expression, got '{type(exprs[i])}'")
+ c_named_expr.expression = (<Expression> exprs[i]).unwrap()
+ if i < len(names):
+ if not isinstance(names[i], str):
+ raise TypeError(f"Expected str, got '{type(names[i])}'")
+ c_named_expr.name = tobytes(<str> names[i])
+ else:
+ c_named_expr.name = tobytes("autoname")
+ c_bound_exprs.named_expressions.push_back(c_named_expr)
+ c_bound_exprs.schema = (<Schema> schema).sp_schema
+
+ c_conversion_options.allow_arrow_extensions = allow_udfs
+
+ c_res_buffer = SerializeExpressions(c_bound_exprs, c_conversion_options)
+ with nogil:
+ c_buffer = GetResultValue(c_res_buffer)
+ return pyarrow_wrap_buffer(c_buffer)
+
+
+cdef class BoundExpressions(_Weakrefable):
+ """
+ A collection of named expressions and the schema they are bound to
+
+ This is equivalent to the Substrait ExtendedExpression message
+ """
+
+ cdef:
+ CBoundExpressions c_bound_exprs
+
+ def __init__(self):
+ msg = 'BoundExpressions is an abstract class thus cannot be initialized.'
+ raise TypeError(msg)
+
+ cdef void init(self, CBoundExpressions bound_expressions):
+ self.c_bound_exprs = bound_expressions
+
+ @property
+ def schema(self):
+ """
+ The common schema that all expressions are bound to
+ """
+ return pyarrow_wrap_schema(self.c_bound_exprs.schema)
+
+ @property
+ def expressions(self):
+ """
+ A dict from expression name to expression
+ """
+ expr_dict = {}
+ for named_expr in self.c_bound_exprs.named_expressions:
+ name = frombytes(named_expr.name)
+ expr = Expression.wrap(named_expr.expression)
+ expr_dict[name] = expr
+ return expr_dict
+
+ @staticmethod
+ cdef wrap(const CBoundExpressions& bound_expressions):
+ cdef BoundExpressions self = BoundExpressions.__new__(BoundExpressions)
+ self.init(bound_expressions)
+ return self
+
+
+def deserialize_expressions(buf):
+ """
+ Deserialize an ExtendedExpression Substrait message into a BoundExpressions object
+
+ Parameters
+ ----------
+ buf : Buffer or bytes
+ The message to deserialize
+
+ Returns
+ -------
+ BoundExpressions
+ The deserialized expressions, their names, and the bound schema
+ """
+ cdef:
+ shared_ptr[CBuffer] c_buffer
+ CResult[CBoundExpressions] c_res_bound_exprs
+ CBoundExpressions c_bound_exprs
+
+ if isinstance(buf, bytes):
+ c_buffer = pyarrow_unwrap_buffer(py_buffer(buf))
+ elif isinstance(buf, Buffer):
+ c_buffer = pyarrow_unwrap_buffer(buf)
+ else:
+ raise TypeError(
+ f"Expected 'pyarrow.Buffer' or bytes, got '{type(buf)}'")
+
+ c_res_bound_exprs = DeserializeExpressions(deref(c_buffer))
+ with nogil:
+ c_bound_exprs = GetResultValue(c_res_bound_exprs)
Review Comment:
I've moved this inside the nogil block.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1256554913
##########
python/pyarrow/_compute.pyx:
##########
@@ -2350,6 +2367,58 @@ cdef class Expression(_Weakrefable):
self.__class__.__name__, str(self)
)
+ @staticmethod
+ def from_substrait(object buffer not None):
+ """
+ Deserialize an expression from Substrait
+
+ The serialized message must be an ExtendedExpression message that has
+ only a single expression. The name of the expression and the schema
+ the expression was bound to will be ignored. Use
+ pyarrow.substrait.deserialize_expressions if this information is needed
+ or if the message might contain multiple expressions.
+
+ Parameters
+ ----------
+ buffer : bytes or Buffer
+ The Substrait message to deserialize
+
+ Returns
+ -------
+ Expression
+ The deserialized expression
+ """
+ expressions = _pas().deserialize_expressions(buffer).expressions
+ if len(expressions) == 0:
+ raise ValueError("Substrait message did not contain any expressions")
+ if len(expressions) > 1:
+ raise ValueError(
+ "Substrait message contained multiple expressions. Use pyarrow.substrait.deserialize_expressions instead")
+ return next(iter(expressions.values()))
+
+ def to_substrait(self, Schema schema not None, c_bool allow_udfs=False):
+ """
+ Serialize the expression using Substrait
+
+ The expression will be serialized as an ExtendedExpression message that has a
+ single expression named "expression"
+
+ Parameters
+ ----------
+ schema : Schema
+ The input schema the expression will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ A buffer containing the serialized Protobuf plan.
+ """
+ return _pas().serialize_expressions([self], "expression", schema, allow_udfs=allow_udfs)
Review Comment:
Oops. This also gets caught now because exprs and names don't have the same length. I've added a test for this as well.
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
Review Comment:
I've changed to this.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1686864354
Yes. Sorry, I have rebased one last time and will merge as soon as CI is green.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1255237904
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -3277,19 +3281,106 @@ def test_expression_serialization():
f = pc.scalar({'a': 1})
g = pc.scalar(pa.scalar(1))
h = pc.scalar(np.int64(2))
+ j = pc.scalar(False)
+
+ literal_exprs = [a, b, c, d, e, g, h, j]
+
+ exprs_with_call = [a == b, a != b, a > b, c & j, c | j, ~c, d.is_valid(),
+ a + b, a - b, a * b, a / b, pc.negate(a),
+ pc.add(a, b), pc.subtract(a, b), pc.divide(a, b),
+ pc.multiply(a, b), pc.power(a, a), pc.sqrt(a),
+ pc.exp(b), pc.cos(b), pc.sin(b), pc.tan(b),
+ pc.acos(b), pc.atan(b), pc.asin(b), pc.atan2(b, b),
+ pc.abs(b), pc.sign(a), pc.bit_wise_not(a),
+ pc.bit_wise_and(a, a), pc.bit_wise_or(a, a),
+ pc.bit_wise_xor(a, a), pc.is_nan(b), pc.is_finite(b),
+ pc.coalesce(a, b),
+ a.cast(pa.int32(), safe=False)]
+
+ exprs_with_ref = [pc.field('i64') > 5, pc.field('i64') == 5,
+ pc.field('i64') == 7,
+ pc.field(('foo', 'bar')) == 'value',
+ pc.field('foo', 'bar') == 'value']
+
+ special_cases = [
+ f, # Struct literals lose their field names
+ a.isin([1, 2, 3]), # isin converts to an or list
+ pc.field('i64').is_null() # pyarrow always specifies a FunctionOptions
+ # for is_null which, being the default, is
+ # dropped on serialization
+ ]
- all_exprs = [a, b, c, d, e, f, g, h, a == b, a > b, a & b, a | b, ~c,
- d.is_valid(), a.cast(pa.int32(), safe=False),
- a.cast(pa.int32(), safe=False), a.isin([1, 2, 3]),
- pc.field('i64') > 5, pc.field('i64') == 5,
- pc.field('i64') == 7, pc.field('i64').is_null(),
- pc.field(('foo', 'bar')) == 'value',
- pc.field('foo', 'bar') == 'value']
+ all_exprs = literal_exprs + exprs_with_call + exprs_with_ref + special_cases
for expr in all_exprs:
assert isinstance(expr, pc.Expression)
restored = pickle.loads(pickle.dumps(expr))
assert expr.equals(restored)
+ if pas is not None:
+
+ test_schema = pa.schema([pa.field("i64", pa.int64()), pa.field(
+ "foo", pa.struct([pa.field("bar", pa.string())]))])
+
+ # Basic literals don't change on binding and so they will round
+ # trip without any change
+ for expr in literal_exprs:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ assert expr.equals(deserialized)
+
+ # Expressions are bound when they get serialized. Since bound
+ # expressions are not equal to their unbound variants we cannot
+ # compare the round tripped with the original
+ for expr in exprs_with_call:
+ serialized = expr.to_substrait(test_schema)
+ deserialized = pc.Expression.from_substrait(serialized)
+ # We can't compare the expressions themselves because of the bound
+ # unbound difference. But we can compare the string representation
Review Comment:
When we bind expressions, in addition to looking up field references, we look up matching kernels (and will actually apply implicit casts) and we store the matching kernel as part of the expression. So "1 + 3" goes from:
```
function: add
args:
1: i64
3: i64
kernel: nullptr
```
...to...
```
function: add
args:
1: i64
3: i64
kernel: add<i64,i64>
```
And unfortunately, this means that Expression::Equals does not compare the two as equal. I've added https://github.com/apache/arrow/issues/36427 to hopefully add that option at some point.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1625634448
I also created https://github.com/substrait-io/substrait/issues/517 on the substrait side in case anyone wants to chime in.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1644572087
I've rebased. The one test failure is a GCS failure and unrelated. Gentle ping for another review.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1694215501
Revision: 4280c4abd9c5d27e392ec4d34f3d28d3d625452b
Submitted crossbow builds: [ursacomputing/crossbow @ actions-44bfd49cd1](https://github.com/ursacomputing/crossbow/branches/all?query=actions-44bfd49cd1)
|Task|Status|
|----|------|
|example-python-minimal-build-fedora-conda|[](https://github.com/ursacomputing/crossbow/actions/runs/5983608140/job/16233922518)|
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1492777718
Leaving as draft as I need to add more test cases as well as python bindings.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1255230483
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ An ExtendedExpression message containing the serialized expressions
+ """
+ cdef:
+ CResult[shared_ptr[CBuffer]] c_res_buffer
+ shared_ptr[CBuffer] c_buffer
+ CNamedExpression c_named_expr
+ CBoundExpressions c_bound_exprs
+ CConversionOptions c_conversion_options
+
+ for i in range(len(exprs)):
+ if not isinstance(exprs[i], Expression):
+ raise TypeError(f"Expected Expression, got '{type(exprs[i])}'")
+ c_named_expr.expression = (<Expression> exprs[i]).unwrap()
+ if i < len(names):
+ if not isinstance(names[i], str):
+ raise TypeError(f"Expected str, got '{type(names[i])}'")
+ c_named_expr.name = tobytes(<str> names[i])
+ else:
+ c_named_expr.name = tobytes("autoname")
Review Comment:
Ok, I was on the fence here. I don't have any real use case for it. I agree it feels better to just give an error if they don't give the same number of names. I'll fix this.
--
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] jorisvandenbossche commented on pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1623688989
> The function `is_in` converts to Substrait's `SingularOrList`. On conversion back to Arrow this becomes an or list. .... This is because Substrait's or list is more expressive ..
And substrait doesn't have an "is_in" like function? (or are there plans for that?)
(this conversion seems unfortunate, as "is_in" can be more efficient than the equivalent or-list)
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1255230124
##########
python/pyarrow/_compute.pyx:
##########
@@ -2350,6 +2367,58 @@ cdef class Expression(_Weakrefable):
self.__class__.__name__, str(self)
)
+ @staticmethod
+ def from_substrait(object buffer not None):
+ """
+ Deserialize an expression from Substrait
+
+ The serialized message must be an ExtendedExpression message that has
+ only a single expression. The name of the expression and the schema
+ the expression was bound to will be ignored. Use
+ pyarrow.substrait.deserialize_expressions if this information is needed
+ or if the message might contain multiple expressions.
+
+ Parameters
+ ----------
+ buffer : bytes or Buffer
+ The Substrait message to deserialize
+
+ Returns
+ -------
+ Expression
+ The deserialized expression
+ """
+ expressions = _pas().deserialize_expressions(buffer).expressions
+ if len(expressions) == 0:
+ raise ValueError("Substrait message did not contain any expressions")
+ if len(expressions) > 1:
+ raise ValueError(
+ "Substrait message contained multiple expressions. Use pyarrow.substrait.deserialize_expressions instead")
+ return next(iter(expressions.values()))
+
+ def to_substrait(self, Schema schema not None, c_bool allow_udfs=False):
+ """
+ Serialize the expression using Substrait
+
+ The expression will be serialized as an ExtendedExpression message that has a
+ single expression named "expression"
+
+ Parameters
+ ----------
+ schema : Schema
+ The input schema the expression will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ A buffer containing the serialized Protobuf plan.
+ """
+ return _pas().serialize_expressions([self], "expression", schema, allow_udfs=allow_udfs)
Review Comment:
Yeah, I was trying to hide the complexity for the to/from substrait methods since the substrait module could always be used for more advanced capabilities.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1256555226
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
Review Comment:
I switched to `allow_arrow_extensions`
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
+ If False then only functions that are part of the core Substrait function
+ definitions will be allowed. Set this to True to allow pyarrow-specific functions
+ but the result may not be accepted by other compute libraries.
+
+ Returns
+ -------
+ Buffer
+ An ExtendedExpression message containing the serialized expressions
+ """
+ cdef:
+ CResult[shared_ptr[CBuffer]] c_res_buffer
+ shared_ptr[CBuffer] c_buffer
+ CNamedExpression c_named_expr
+ CBoundExpressions c_bound_exprs
+ CConversionOptions c_conversion_options
+
+ for i in range(len(exprs)):
+ if not isinstance(exprs[i], Expression):
+ raise TypeError(f"Expected Expression, got '{type(exprs[i])}'")
+ c_named_expr.expression = (<Expression> exprs[i]).unwrap()
+ if i < len(names):
+ if not isinstance(names[i], str):
+ raise TypeError(f"Expected str, got '{type(names[i])}'")
+ c_named_expr.name = tobytes(<str> names[i])
+ else:
+ c_named_expr.name = tobytes("autoname")
+ c_bound_exprs.named_expressions.push_back(c_named_expr)
+ c_bound_exprs.schema = (<Schema> schema).sp_schema
+
+ c_conversion_options.allow_arrow_extensions = allow_udfs
+
+ c_res_buffer = SerializeExpressions(c_bound_exprs, c_conversion_options)
+ with nogil:
+ c_buffer = GetResultValue(c_res_buffer)
Review Comment:
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] github-actions[bot] commented on pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1492777661
:warning: GitHub issue #33985 **has been automatically assigned in GitHub** to PR creator.
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1549144409
I've added python bindings. Now all that is needed is documentation / examples
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1615973586
I'm marking this ready for review. I still want to add a few unit tests that verify we correctly raise errors when given Substrait expressions that Arrow cannot handle (e.g. MultiOrList) but these should be a minor addition.
--
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] ianmcook commented on pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1683997774
@westonpace @jorisvandenbossche is this ready to merge?
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1688542273
Failures appear 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 merged pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace merged PR #34834:
URL: https://github.com/apache/arrow/pull/34834
--
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] kou commented on pull request #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34834:
URL: https://github.com/apache/arrow/pull/34834#issuecomment-1694214975
@github-actions crossbow submit example-python-minimal-build-fedora-conda
--
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 #34834: GH-33985: [C++] Add substrait serialization/deserialization for expressions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34834:
URL: https://github.com/apache/arrow/pull/34834#discussion_r1255233281
##########
python/pyarrow/_substrait.pyx:
##########
@@ -185,6 +187,143 @@ def _parse_json_plan(plan):
return pyarrow_wrap_buffer(c_buf_plan)
+def serialize_expressions(exprs, names, schema, *, allow_udfs=False):
+ """
+ Serialize a collection of expressions into Substrait
+
+ Substrait expressions must be bound to a schema. For example,
+ the Substrait expression ``a_i32 + b_i32`` is different from the
+ Substrait expression ``a_i64 + b_i64``. Pyarrow expressions are
+ typically unbound. For example, both of the above expressions
+ would be represented as ``a + b`` in pyarrow.
+
+ This means a schema must be provided when serializing an expression.
+ It also means that the serialization may fail if a matching function
+ call cannot be found for the expression.
+
+ Parameters
+ ----------
+ exprs : list of Expression
+ The expressions to serialize
+ names : list of str
+ Names for the expressions
+ schema : Schema
+ The schema the expressions will be bound to
+ allow_udfs : bool, default False
Review Comment:
Ah, I forgot I needed to reconcile this :) Both made sense to me.
Setting this to true enables both "arrow builtin functions that are not substrait functions" and "user registered udfs (or actual UDFs)" (from Substrait's perspective these are the same thing). My thinking was that, as Substrait's function support expands, the first case wouldn't be encountered as much. However, I think both names are still ok. I'm happy to switch to "allow_arrow_extensions"
--
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