ARROW-7288: [C++][Parquet] Don't use regular expression to parse application version #9367
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nealrichardson
approved these changes
Jan 30, 2021
There was a problem hiding this comment.
I like this change. It seemed like overkill to require a regex library only to handle parsing a version string.
I guess there is some risk that there is some funky file out there that we'll fail to parse its version string, but (1) in our code we only seem to care about the major.minor.patch version, which is trivial to parse and covered in tests here, and (2) parquet-mr hasn't changed its code for writing version strings since 2015, and that was just to add prerelease info (which we never check after parsing). So it seems safe to me.
|
@github-actions crossbow submit -g nightly |
|
Revision: 076863b Submitted crossbow builds: ursacomputing/crossbow @ actions-59
|
pitrou
requested changes
Feb 1, 2021
| ASSERT_EQ("cdh5.5.0", version.version.pre_release); | ||
| ASSERT_EQ("cd", version.version.build_info); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you add a test with a malformed version string?
There was a problem hiding this comment.
I've add some more tests.
Note that this implementation assumes that input encoding is ASCII. It may not work with other encodings.
Should we support non ASCII encodings?
cpp/src/parquet/metadata.cc
Outdated
| private: | ||
| void RemovePrecedingSpaces(const std::string& string, size_t& start, | ||
| const size_t& end) { | ||
| while (start < end && string[start] == ' ') { |
There was a problem hiding this comment.
Note that \s in a regex may match other whitespace characters. But they're unlikely to appear in the created_by field anyway.
There was a problem hiding this comment.
I've added \t, \v, \r, \n and \f to supported whitespace characters but they will not be appeared as you said.
…ication version std::regex provided by MinGW may take a long with Japanese location on Windows. We can use std::regex, boost::regex or RE2 as regular expression engine for this but RE2 doesn't use compatible syntax with others. If we support all of them, we need to maintain multiple regular expressions. It increases maintenance cost. If we don't use regular expression, we don't need to think about regular expression. But we need to maintain hand-written parser.
kou
force-pushed
the
cpp-parquet-no-regex
branch
from
076863b
to
c85da69
Compare
pitrou
approved these changes
Feb 2, 2021
|
Travis-CI build: https://travis-ci.com/github/pitrou/arrow/builds/215736951 |
|
Thanks for the boolean fix! |
nevi-me
pushed a commit
to nevi-me/arrow
that referenced
this pull request
Feb 13, 2021
…ication version std::regex provided by MinGW may take a long with Japanese location on Windows. We can use std::regex, boost::regex or RE2 as regular expression engine for this but RE2 doesn't use compatible syntax with others. If we support all of them, we need to maintain multiple regular expressions. It increases maintenance cost. If we don't use regular expression, we don't need to think about regular expression. But we need to maintain hand-written parser. Closes apache#9367 from kou/cpp-parquet-no-regex Lead-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
GeorgeAp
pushed a commit
to sirensolutions/arrow
that referenced
this pull request
Jun 7, 2021
…ication version std::regex provided by MinGW may take a long with Japanese location on Windows. We can use std::regex, boost::regex or RE2 as regular expression engine for this but RE2 doesn't use compatible syntax with others. If we support all of them, we need to maintain multiple regular expressions. It increases maintenance cost. If we don't use regular expression, we don't need to think about regular expression. But we need to maintain hand-written parser. Closes apache#9367 from kou/cpp-parquet-no-regex Lead-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
michalursa
pushed a commit
to michalursa/arrow
that referenced
this pull request
Jun 13, 2021
…ication version std::regex provided by MinGW may take a long with Japanese location on Windows. We can use std::regex, boost::regex or RE2 as regular expression engine for this but RE2 doesn't use compatible syntax with others. If we support all of them, we need to maintain multiple regular expressions. It increases maintenance cost. If we don't use regular expression, we don't need to think about regular expression. But we need to maintain hand-written parser. Closes apache#9367 from kou/cpp-parquet-no-regex Lead-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
std::regex provided by MinGW may take a long with Japanese location on
Windows.
We can use std::regex, boost::regex or RE2 as regular expression
engine for this but RE2 doesn't use compatible syntax with others. If
we support all of them, we need to maintain multiple regular
expressions. It increases maintenance cost. If we don't use regular
expression, we don't need to think about regular expression. But we
need to maintain hand-written parser.