You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/01 04:57:15 UTC
[GitHub] [arrow] save-buffer opened a new pull request, #14287: Implement a parser for Expressions
save-buffer opened a new pull request, #14287:
URL: https://github.com/apache/arrow/pull/14287
I've seen a few people on the mailing list asking for something like this and I've wanted it myself, so I went ahead and implemented a parser for a lisp-like way of generating Expressions. Calls are of the form `(<fn> <args>)`, scalars are of the form `$type:value`, and field refs are of the form `!<dot path>`.
--
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 #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1404358168
There is a discussion on the Substrait mailing list about defining an expression language as part of a text serialization format for Substrait: https://groups.google.com/g/substrait/c/iCiQR-tHI4Q/m/slzrzdcQAgAJ
Substrait seems like a more appropriate and sustainable place to define an expression language, maintain and version it over time, handle forward and backward compatibility considerations across versions, etc. Of course we will still need Arrow libraries to implement parsers for the expression language. Could the work in this PR be adapted to parse expressions in a language along the lines of what is proposed in that thread on the Substrait mailing 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] save-buffer commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1029943869
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
Changed 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] save-buffer commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1029944054
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
Sounds good, thanks for the clarification Antoine. I've changed it to take a pointer.
--
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] aucahuasi commented on pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
aucahuasi commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1326895583
LGTM! For the next PRs, would be great to invest a bit to improve even more the error handling: to provide more context to the user in the case of invalid expressions.
For instance, for this invalid expression:
`add($duration(MILLI):10, $duration(MILLIa):20)`
we get this error:
`'_error_or_value82.status()' failed with Invalid: Error at index 43: Unterminated data type arg list!`
it would be nice to get a more precise error message here.
btw I built this on a macbook pro m1 without issues (clang compiler)
--
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 #14287: Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1264243196
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs 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 pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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] save-buffer commented on pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1324481193
Added better error handling and support for escaping stuff with "\".
--
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] save-buffer commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1026094560
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
I don't like anonymous namespaces because they are extremely debugger-hostile. I actually have an open issue to either disable anonymous namespaces in debug mode or to switch everything to using `static` everywhere.
--
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] save-buffer commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1026097946
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
+ constexpr const char* kWhitespaces = " \f\n\r\t\v";
+ size_t first_nonwhitespace = view.find_first_not_of(kWhitespaces);
+ view.remove_prefix(first_nonwhitespace);
+}
+
+static std::string_view ExtractUntil(std::string_view& view,
+ const std::string_view separators) {
+ size_t separator = view.find_first_of(separators);
+ std::string_view result = view.substr(0, separator);
+ view.remove_prefix(separator);
+ return result;
+}
+
+static std::string_view TrimUntilNextSeparator(std::string_view& view) {
+ constexpr const char* separators = " \f\n\r\t\v),";
+ return ExtractUntil(view, separators);
+}
+
+static std::string_view ExtractArgument(std::string_view& view) {
+ constexpr const char* separators = ",)";
+ return ExtractUntil(view, separators);
+}
+
+static const std::unordered_map<std::string_view, std::shared_ptr<DataType>>
+ kNameToSimpleType = {
+ {"null", null()},
+ {"boolean", boolean()},
+ {"int8", int8()},
+ {"int16", int16()},
+ {"int32", int32()},
+ {"int64", int64()},
+ {"uint8", uint8()},
+ {"uint16", uint16()},
+ {"uint32", uint32()},
+ {"uint64", uint64()},
+ {"float16", float16()},
+ {"float32", float32()},
+ {"float64", float64()},
+ {"utf8", utf8()},
+ {"large_utf8", large_utf8()},
+ {"binary", binary()},
+ {"large_binary", large_binary()},
+ {"date32", date32()},
+ {"date64", date64()},
+ {"day_time_interval", day_time_interval()},
+ {"month_interval", month_interval()},
+ {"month_day_nano_interval", month_day_nano_interval()},
+};
+
+static Result<std::shared_ptr<DataType>> ParseDataType(std::string_view& type);
+
+// Takes the args list not including the enclosing parentheses
+using InstantiateTypeFn =
+ std::add_pointer_t<Result<std::shared_ptr<DataType>>(std::string_view&)>;
+
+static Result<int32_t> ParseInt32(std::string_view& args) {
+ ConsumeWhitespace(args);
+ int32_t result;
+ auto [finish, ec] = std::from_chars(args.data(), args.data() + args.size(), result);
+ if (ec == std::errc::invalid_argument)
+ return Status::Invalid("Could not parse ", args, " as an int32!");
+
+ args.remove_prefix(finish - args.data());
+ return result;
+}
+
+static Status ParseComma(std::string_view& args) {
+ ConsumeWhitespace(args);
+ if (args.empty() || args[0] != ',')
+ return Status::Invalid("Expected comma-separated args list near ", args);
+ args.remove_prefix(1);
+ return Status::OK();
+}
+
+static Result<std::shared_ptr<DataType>> ParseFixedSizeBinary(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(int32_t byte_width, ParseInt32(args));
+ return fixed_size_binary(byte_width);
+}
+
+static Result<std::pair<int32_t, int32_t>> ParseDecimalArgs(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(int32_t precision, ParseInt32(args));
+ RETURN_NOT_OK(ParseComma(args));
+ ARROW_ASSIGN_OR_RAISE(int32_t scale, ParseInt32(args));
+ return std::pair<int32_t, int32_t>(precision, scale);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDecimal(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(auto ps, ParseDecimalArgs(args));
+ return decimal(ps.first, ps.second);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDecimal128(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(auto ps, ParseDecimalArgs(args));
+ return decimal128(ps.first, ps.second);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDecimal256(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(auto ps, ParseDecimalArgs(args));
+ return decimal256(ps.first, ps.second);
+}
+
+static Result<std::shared_ptr<DataType>> ParseList(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> list_type, ParseDataType(args));
+ return list(std::move(list_type));
+}
+
+static Result<std::shared_ptr<DataType>> ParseLargeList(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> list_type, ParseDataType(args));
+ return large_list(std::move(list_type));
+}
+
+static Result<std::shared_ptr<DataType>> ParseMap(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> key_type, ParseDataType(args));
+ RETURN_NOT_OK(ParseComma(args));
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> value_type, ParseDataType(args));
+ return map(std::move(key_type), std::move(value_type));
+}
+
+static Result<std::shared_ptr<DataType>> ParseFixedSizeList(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> list_type, ParseDataType(args));
+ RETURN_NOT_OK(ParseComma(args));
+ ARROW_ASSIGN_OR_RAISE(int32_t size, ParseInt32(args));
+ return fixed_size_list(std::move(list_type), size);
+}
+
+static Result<TimeUnit::type> ParseTimeUnit(std::string_view& args) {
+ ConsumeWhitespace(args);
+ if (args.empty()) return Status::Invalid("Expected a time unit near ", args);
+
+ const std::string_view options[4] = {"SECOND", "MILLI", "MICRO", "NANO"};
+ for (size_t i = 0; i < 4; i++) {
+ if (args.find(options[i]) == 0) {
+ args.remove_prefix(options[i].size());
+ return TimeUnit::values()[i];
+ }
+ }
+ return Status::Invalid("Unrecognized TimeUnit ", args);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDuration(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(TimeUnit::type unit, ParseTimeUnit(args));
+ return duration(unit);
+}
+
+static Result<std::shared_ptr<DataType>> ParseTimestamp(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(TimeUnit::type unit, ParseTimeUnit(args));
+ return timestamp(unit);
+}
+
+static Result<std::shared_ptr<DataType>> ParseTime32(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(TimeUnit::type unit, ParseTimeUnit(args));
+ return time32(unit);
+}
+
+static Result<std::shared_ptr<DataType>> ParseTime64(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(TimeUnit::type unit, ParseTimeUnit(args));
+ return time64(unit);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDictionary(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> key_type, ParseDataType(args));
+ RETURN_NOT_OK(ParseComma(args));
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> value_type, ParseDataType(args));
+ return dictionary(std::move(key_type), std::move(value_type));
+}
+
+static const std::unordered_map<std::string_view, InstantiateTypeFn>
+ kNameToParameterizedType = {
+ {"fixed_size_binary", ParseFixedSizeBinary},
+ {"decimal", ParseDecimal},
+ {"decimal128", ParseDecimal128},
+ {"decimal256", ParseDecimal256},
+ {"list", ParseList},
+ {"large_list", ParseLargeList},
+ {"map", ParseMap},
+ {"fixed_size_list", ParseFixedSizeList},
+ {"duration", ParseDuration},
+ {"timestamp", ParseTimestamp},
+ {"time32", ParseTime32},
+ {"time64", ParseTime64},
+ {"dictionary", ParseDictionary},
+};
+
+static Result<Expression> ParseExpr(std::string_view& expr);
+
+static Result<Expression> ParseCall(std::string_view& expr) {
+ ConsumeWhitespace(expr);
+ if (expr.empty()) return Status::Invalid("Found empty expression");
+
+ std::string_view function_name = ExtractUntil(expr, "(");
+ if (expr.empty())
+ return Status::Invalid("Expected argument list after function name", function_name);
+ expr.remove_prefix(1); // Remove the open paren
+
+ std::vector<Expression> args;
+ do {
+ ConsumeWhitespace(expr);
+ if (expr.empty())
+ return Status::Invalid("Found unterminated expression argument list");
+ if (expr[0] == ')') break;
+ if (!args.empty()) RETURN_NOT_OK(ParseComma(expr));
+
+ ARROW_ASSIGN_OR_RAISE(Expression arg, ParseExpr(expr));
+ args.emplace_back(std::move(arg));
+ } while (true);
+
+ expr.remove_prefix(1); // Remove the close paren
+ return call(std::string(function_name), std::move(args));
+}
+
+static Result<Expression> ParseFieldRef(std::string_view& expr) {
+ if (expr.empty()) return Status::Invalid("Found an empty named fieldref");
+
+ std::string_view dot_path = ExtractArgument(expr);
Review Comment:
It should, `find_first_of` will return `npos`, and then the `substr` from `0` to `npos` will return the full string.
--
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] zinking commented on pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by "zinking (via GitHub)" <gi...@apache.org>.
zinking commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1657993149
> There is a discussion on the Substrait mailing list about defining an expression language as part of a text serialization format for Substrait: https://groups.google.com/g/substrait/c/iCiQR-tHI4Q/m/slzrzdcQAgAJ
>
> Substrait seems like a more appropriate and sustainable place to define an expression language, maintain and version it over time, handle forward and backward compatibility considerations across versions, etc. Of course we will still need Arrow libraries to implement parsers for the expression language. Could the work in this PR be adapted to parse expressions in a language along the lines of what is proposed in that thread on the Substrait mailing list?
while the substrait design might be the correct direction to go, but I feel that's a much broader scope compared with this. this PR could bring the preliminaries of filtering into arrow, so some user requests could be fulfilled.
and when the substrait integration is mature and complete, this can be switched at that point.
all in all, folks, @ianmcook @amol- @westonpace any chance this gets revived and get merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1027063186
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
By the way this should be `void ConsumeWhitespace(std::string_view* view)` since the parameter is mutable.
--
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] save-buffer commented on pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1281333133
@pitrou @westonpace I've updated the parser to reflect the discussion on the mailing list. The language now looks like the traditional function call syntax instead of the lisp-style syntax. I've also gotten rid of the `!` needed before FieldRefs (now it's either `.name` or `[idx]`). I've kept the syntax for literals the same
```
add(.a, $int32:1)
```
--
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 #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1273754188
https://issues.apache.org/jira/browse/ARROW-17351
--
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] save-buffer commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1026962456
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
Is this a guideline we currently have? The Google guide says:
```
All declarations can be given internal linkage by placing them in unnamed namespaces. Functions and variables can also be given internal linkage by declaring them static
```
So it seems that either way works according to the current guidelines, in which case I'd prefer to maintain compatibility with debuggers.
--
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 #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1026949599
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
Until that goes through though I think we will have to stick to the current guidelines.
--
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] amol- commented on pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1490649788
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
--
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 #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1025769380
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
The arrow style guide is to use pointers for mutable inputs (e.g. not mutable references) as it makes it more obvious at the call site that the value you are passing in will be modified.
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
+ constexpr const char* kWhitespaces = " \f\n\r\t\v";
+ size_t first_nonwhitespace = view.find_first_not_of(kWhitespaces);
+ view.remove_prefix(first_nonwhitespace);
+}
+
+static std::string_view ExtractUntil(std::string_view& view,
+ const std::string_view separators) {
+ size_t separator = view.find_first_of(separators);
+ std::string_view result = view.substr(0, separator);
+ view.remove_prefix(separator);
+ return result;
+}
+
+static std::string_view TrimUntilNextSeparator(std::string_view& view) {
+ constexpr const char* separators = " \f\n\r\t\v),";
+ return ExtractUntil(view, separators);
+}
+
+static std::string_view ExtractArgument(std::string_view& view) {
+ constexpr const char* separators = ",)";
+ return ExtractUntil(view, separators);
+}
+
+static const std::unordered_map<std::string_view, std::shared_ptr<DataType>>
+ kNameToSimpleType = {
+ {"null", null()},
+ {"boolean", boolean()},
+ {"int8", int8()},
+ {"int16", int16()},
+ {"int32", int32()},
+ {"int64", int64()},
+ {"uint8", uint8()},
+ {"uint16", uint16()},
+ {"uint32", uint32()},
+ {"uint64", uint64()},
+ {"float16", float16()},
+ {"float32", float32()},
+ {"float64", float64()},
+ {"utf8", utf8()},
+ {"large_utf8", large_utf8()},
+ {"binary", binary()},
+ {"large_binary", large_binary()},
+ {"date32", date32()},
+ {"date64", date64()},
+ {"day_time_interval", day_time_interval()},
+ {"month_interval", month_interval()},
+ {"month_day_nano_interval", month_day_nano_interval()},
+};
+
+static Result<std::shared_ptr<DataType>> ParseDataType(std::string_view& type);
+
+// Takes the args list not including the enclosing parentheses
+using InstantiateTypeFn =
+ std::add_pointer_t<Result<std::shared_ptr<DataType>>(std::string_view&)>;
+
+static Result<int32_t> ParseInt32(std::string_view& args) {
+ ConsumeWhitespace(args);
+ int32_t result;
+ auto [finish, ec] = std::from_chars(args.data(), args.data() + args.size(), result);
+ if (ec == std::errc::invalid_argument)
+ return Status::Invalid("Could not parse ", args, " as an int32!");
+
+ args.remove_prefix(finish - args.data());
+ return result;
+}
+
+static Status ParseComma(std::string_view& args) {
+ ConsumeWhitespace(args);
+ if (args.empty() || args[0] != ',')
+ return Status::Invalid("Expected comma-separated args list near ", args);
+ args.remove_prefix(1);
+ return Status::OK();
+}
+
+static Result<std::shared_ptr<DataType>> ParseFixedSizeBinary(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(int32_t byte_width, ParseInt32(args));
+ return fixed_size_binary(byte_width);
+}
+
+static Result<std::pair<int32_t, int32_t>> ParseDecimalArgs(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(int32_t precision, ParseInt32(args));
+ RETURN_NOT_OK(ParseComma(args));
+ ARROW_ASSIGN_OR_RAISE(int32_t scale, ParseInt32(args));
+ return std::pair<int32_t, int32_t>(precision, scale);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDecimal(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(auto ps, ParseDecimalArgs(args));
+ return decimal(ps.first, ps.second);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDecimal128(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(auto ps, ParseDecimalArgs(args));
+ return decimal128(ps.first, ps.second);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDecimal256(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(auto ps, ParseDecimalArgs(args));
+ return decimal256(ps.first, ps.second);
+}
+
+static Result<std::shared_ptr<DataType>> ParseList(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> list_type, ParseDataType(args));
+ return list(std::move(list_type));
+}
+
+static Result<std::shared_ptr<DataType>> ParseLargeList(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> list_type, ParseDataType(args));
+ return large_list(std::move(list_type));
+}
+
+static Result<std::shared_ptr<DataType>> ParseMap(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> key_type, ParseDataType(args));
+ RETURN_NOT_OK(ParseComma(args));
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> value_type, ParseDataType(args));
+ return map(std::move(key_type), std::move(value_type));
+}
+
+static Result<std::shared_ptr<DataType>> ParseFixedSizeList(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> list_type, ParseDataType(args));
+ RETURN_NOT_OK(ParseComma(args));
+ ARROW_ASSIGN_OR_RAISE(int32_t size, ParseInt32(args));
+ return fixed_size_list(std::move(list_type), size);
+}
+
+static Result<TimeUnit::type> ParseTimeUnit(std::string_view& args) {
+ ConsumeWhitespace(args);
+ if (args.empty()) return Status::Invalid("Expected a time unit near ", args);
+
+ const std::string_view options[4] = {"SECOND", "MILLI", "MICRO", "NANO"};
+ for (size_t i = 0; i < 4; i++) {
+ if (args.find(options[i]) == 0) {
+ args.remove_prefix(options[i].size());
+ return TimeUnit::values()[i];
+ }
+ }
+ return Status::Invalid("Unrecognized TimeUnit ", args);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDuration(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(TimeUnit::type unit, ParseTimeUnit(args));
+ return duration(unit);
+}
+
+static Result<std::shared_ptr<DataType>> ParseTimestamp(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(TimeUnit::type unit, ParseTimeUnit(args));
+ return timestamp(unit);
+}
+
+static Result<std::shared_ptr<DataType>> ParseTime32(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(TimeUnit::type unit, ParseTimeUnit(args));
+ return time32(unit);
+}
+
+static Result<std::shared_ptr<DataType>> ParseTime64(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(TimeUnit::type unit, ParseTimeUnit(args));
+ return time64(unit);
+}
+
+static Result<std::shared_ptr<DataType>> ParseDictionary(std::string_view& args) {
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> key_type, ParseDataType(args));
+ RETURN_NOT_OK(ParseComma(args));
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<DataType> value_type, ParseDataType(args));
+ return dictionary(std::move(key_type), std::move(value_type));
+}
+
+static const std::unordered_map<std::string_view, InstantiateTypeFn>
+ kNameToParameterizedType = {
+ {"fixed_size_binary", ParseFixedSizeBinary},
+ {"decimal", ParseDecimal},
+ {"decimal128", ParseDecimal128},
+ {"decimal256", ParseDecimal256},
+ {"list", ParseList},
+ {"large_list", ParseLargeList},
+ {"map", ParseMap},
+ {"fixed_size_list", ParseFixedSizeList},
+ {"duration", ParseDuration},
+ {"timestamp", ParseTimestamp},
+ {"time32", ParseTime32},
+ {"time64", ParseTime64},
+ {"dictionary", ParseDictionary},
+};
+
+static Result<Expression> ParseExpr(std::string_view& expr);
+
+static Result<Expression> ParseCall(std::string_view& expr) {
+ ConsumeWhitespace(expr);
+ if (expr.empty()) return Status::Invalid("Found empty expression");
+
+ std::string_view function_name = ExtractUntil(expr, "(");
+ if (expr.empty())
+ return Status::Invalid("Expected argument list after function name", function_name);
+ expr.remove_prefix(1); // Remove the open paren
+
+ std::vector<Expression> args;
+ do {
+ ConsumeWhitespace(expr);
+ if (expr.empty())
+ return Status::Invalid("Found unterminated expression argument list");
+ if (expr[0] == ')') break;
+ if (!args.empty()) RETURN_NOT_OK(ParseComma(expr));
+
+ ARROW_ASSIGN_OR_RAISE(Expression arg, ParseExpr(expr));
+ args.emplace_back(std::move(arg));
+ } while (true);
+
+ expr.remove_prefix(1); // Remove the close paren
+ return call(std::string(function_name), std::move(args));
+}
+
+static Result<Expression> ParseFieldRef(std::string_view& expr) {
+ if (expr.empty()) return Status::Invalid("Found an empty named fieldref");
+
+ std::string_view dot_path = ExtractArgument(expr);
Review Comment:
Does `ExtractArgument` work if the entire expression is a field ref (e.g. `.x`)
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
Let's put everything inside an anonymous namespace instead of using static.
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
+ constexpr const char* kWhitespaces = " \f\n\r\t\v";
+ size_t first_nonwhitespace = view.find_first_not_of(kWhitespaces);
+ view.remove_prefix(first_nonwhitespace);
+}
+
+static std::string_view ExtractUntil(std::string_view& view,
+ const std::string_view separators) {
+ size_t separator = view.find_first_of(separators);
+ std::string_view result = view.substr(0, separator);
+ view.remove_prefix(separator);
+ return result;
Review Comment:
```suggestion
std::string_view extracted = view.substr(0, separator);
view.remove_prefix(separator);
return extracted;
```
--
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 #14287: ARROW-17906: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1264243397
:warning: Ticket **has no components in JIRA**, make sure you assign one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1027063159
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
Review Comment:
You can make it `static` if you prefer. Generally we use the anonymous namespace since it's easy to forget the `static` keyword in front of a helper, and it doesn't work for non-functions.
--
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 #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1066368953
##########
cpp/src/arrow/compute/exec/expression.h:
##########
@@ -129,6 +129,20 @@ class ARROW_EXPORT Expression {
explicit Expression(Datum literal);
explicit Expression(Parameter parameter);
+ /*
+ Grammar:
+ Expr -> FieldRef | Literal | Call
+
+ FieldRef -> Field | Field FieldRef
+ Field -> . Name | [ Number ]
+
+ Literal -> $ TypeName : Value
Review Comment:
This is missing rules for escaping right? I think those need to be part of the grammar.
--
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 #14287: ARROW-17906: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1264243393
https://issues.apache.org/jira/browse/ARROW-17906
--
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] NoahFournier commented on pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
NoahFournier commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1353124498
Hey there all, I'm very interested in getting this PR merged ASAP. Is there any remaining work on this to get it merged?
--
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] amol- closed pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
URL: https://github.com/apache/arrow/pull/14287
--
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] danepitkin commented on pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1714703792
Hi @zinking , I don't think there is any plan to revive this.
FWIW, the Substrait ExtendedExpression support landed in C++ and Python[1]. The Java implementation[2] is in final review stages as well. I believe this is the current preferred approach.
[1] https://github.com/apache/arrow/pull/34834
[2] https://github.com/apache/arrow/pull/35570
--
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] aucahuasi commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
aucahuasi commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1022169096
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
Review Comment:
Perhaps the parser looks simple now, but I would like to ask if you had considered using a parser generator instead (for future maintainability)?
--
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] save-buffer commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1029943933
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cctype>
+#include <charconv>
+#include <string_view>
+#include <type_traits>
+#include <unordered_map>
+
+#include "arrow/compute/exec/expression.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow {
+namespace compute {
+static void ConsumeWhitespace(std::string_view& view) {
+ constexpr const char* kWhitespaces = " \f\n\r\t\v";
+ size_t first_nonwhitespace = view.find_first_not_of(kWhitespaces);
+ view.remove_prefix(first_nonwhitespace);
+}
+
+static std::string_view ExtractUntil(std::string_view& view,
+ const std::string_view separators) {
+ size_t separator = view.find_first_of(separators);
+ std::string_view result = view.substr(0, separator);
+ view.remove_prefix(separator);
+ return result;
Review Comment:
I like the name `prefix` more, so I switched it to that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a diff in pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #14287:
URL: https://github.com/apache/arrow/pull/14287#discussion_r1024649711
##########
cpp/src/arrow/compute/exec/parsing.cc:
##########
@@ -0,0 +1,311 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
Review Comment:
I don't think that parser generators are more maintainable. They require learning a new syntax, adding an extra build step, adding an extra dependency to the project, and require debugging generated code. In my experience writing parsers, it's always fewer lines of code and simpler to use to hand-write the parser.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1353345864
@NoahFournier Sorry. The project is lacking review bandwidth at the moment, so we have to prioritize work and this might unfortunately take some time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #14287: ARROW-17351: [C++][Compute] Implement a parser for Expressions
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #14287:
URL: https://github.com/apache/arrow/pull/14287#issuecomment-1373020186
I'd like to revive this as it has been an ask for some time and I think it is important. The technical issues of how the parser is created are probably more minor than the maintenance issue of making sure we come up with an expression syntax we are willing to support and expect to last.
There was a ML discussion on this but I feel it stalled out somewhat. Part of the challenge is that there were two alternatives proposed. Another challenge is that it would be unfortunate to adopt one standard in Arrow only to have Substrait adopt a different standard later. I propose the following:
* Build up a corpus of example expressions (10-20 or so) that demonstrate the various features (different types of scalars, escaping strings, etc.)
* Create a grammar for all proposals (I believe this will help when communicating)
* Send a message to the Substrait mailing list with the proposal
* Revive the Arrow ML discussion and point any interested parties to the Substrait discussion
* Once the Substrait discussion reaches consensus we can merge a parser into arrow-c++
--
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