s3select: add trim operator , fix substring and bypass boost::spirit rescan by albin-antony · Pull Request #39 · ceph/s3select

albin-antony · 2020-12-01T09:29:06Z

Align ceph s3select with aws.

Signed-off-by: Albin Antony aantony@redhat.com

galsalomon66 · 2020-12-02T20:14:11Z

include/s3select.h

+
+      substr_one_arg = (bsc::str_p("substring") >> '(' >> (arithmetic_expression >> bsc::str_p(",") >> arithmetic_expression) >> ')') [BOOST_BIND_ACTION(push_substr_one_arg)];
+
+      substr_two_arg = (bsc::str_p("substring") >> '(' >> (arithmetic_expression >> bsc::str_p(",") >> arithmetic_expression >> bsc::str_p(",") >> arithmetic_expression) >> ')') [BOOST_BIND_ACTION(push_substr_two_arg)];


no need for substr_one_arg / substr_two_arg rules.
the rule for function accept both rules , the operator will handle those different functions.

Argument order reverses for push_substr_from and push_substr_from_for in operator. I have modified substring operator to accommodate that. Should I create new operator for 'from' and 'from_for'?

it better to do it in single operator (re-use code).
main consideration is performance.
the /operator()/ is very intensive , thus code should be compact as possible.

galsalomon66 · 2020-12-04T09:17:54Z

include/s3select_functions.h

    {"float", s3select_func_En_t::TO_FLOAT},
-    {"substr", s3select_func_En_t::SUBSTR},
+    {"substring", s3select_func_En_t::SUBSTR},
+    {"substring_from", s3select_func_En_t::SUBSTR_FROM},


must "hide" those functions with '#' ( "#substring_from#")

galsalomon66 · 2020-12-04T09:24:00Z

include/s3select.h

+  self->getAction()->exprQ.pop_back();
+  func->push_argument(inp_expr);
+
+  base_statement* inp_exp = self->getAction()->exprQ.back();


typo:
use better name for variables , it makes the code more readable
in this case, it will describe the order of parameters and their roles.

galsalomon66 · 2020-12-04T09:34:02Z

include/s3select_functions.h

      iter++;
      to = *iter;
+      v_to = to->eval();
+      if (v_to.is_string())


better to use /is_number()/

galsalomon66 · 2020-12-04T09:34:48Z

include/s3select_functions.h

+      iter++;
+      str = *iter;
      v_to = to->eval();
      if (v_to.is_string())


better to use /is_number()/

galsalomon66 · 2020-12-04T09:35:24Z

include/s3select_functions.h

+    int str_length = strlen(v_str.str());
+
+    v_from = from->eval();
+    if(v_from.is_string())


better to use /is_number()/

galsalomon66 · 2020-12-04T09:37:37Z

include/s3select.h

+
+      substr_from = (bsc::str_p("substring") >> '(' >> (arithmetic_expression >> bsc::str_p("from") >> arithmetic_expression) >> ')') [BOOST_BIND_ACTION(push_substr_from)];
+
+      substr_from_for = (bsc::str_p("substring") >> '(' >> (arithmetic_expression >> bsc::str_p("from") >> arithmetic_expression >> bsc::str_p("for") >> arithmetic_expression) >> ')') [BOOST_BIND_ACTION(push_substr_from_for)];


ok,
those rules accepts the uniq syntax for substring function.

galsalomon66 · 2020-12-04T09:43:09Z

looks good.
i added some comments.

albin-antony · 2020-12-04T10:51:19Z

Thanks. I have addressed the comments.

galsalomon66 · 2020-12-05T09:33:41Z

substr::operator() and substr_from::operator
are actually identical , as should (and duplicated).
it seems that order of parameters is the only difference, and this can be arrange.
duplicate code , is error-prone.
one solution could be, that both operators will inherit from the same base.

… to bind-action more than once per the same text) Signed-off-by: gal salomon <gal.salomon@gmail.com>

Align ceph s3select with aws. Signed-off-by: Albin Antony <aantony@redhat.com>

galsalomon66 · 2020-12-16T08:18:41Z

include/s3select.h

+  }
+} 
+
+void push_trim_remove_type::builder(s3select* self, const char* a, const char* b) const


push_trim_type identical to push_trim_remove_type

Thanks! I have missed it

Signed-off-by: Albin Antony <aantony@redhat.com>

galsalomon66

good coverage

albin-antony force-pushed the substr-fix branch 2 times, most recently from 35dc415 to c5caf63 Compare December 2, 2020 10:15

galsalomon66 reviewed Dec 2, 2020

View reviewed changes

albin-antony force-pushed the substr-fix branch 2 times, most recently from 011396e to 7c5daee Compare December 3, 2020 10:50

albin-antony requested a review from galsalomon66 December 3, 2020 10:51

galsalomon66 reviewed Dec 4, 2020

View reviewed changes

include/s3select_functions.h Outdated

iter++;

to = *iter;

v_to = to->eval();

if (v_to.is_string())

Copy link

galsalomon66 Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to use /is_number()/

galsalomon66 reviewed Dec 4, 2020

View reviewed changes

include/s3select_functions.h Outdated

iter++;

str = *iter;

v_to = to->eval();

if (v_to.is_string())

Copy link

galsalomon66 Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to use /is_number()/

galsalomon66 reviewed Dec 4, 2020

View reviewed changes

albin-antony force-pushed the substr-fix branch from 7c5daee to 319865c Compare December 4, 2020 10:48

albin-antony mentioned this pull request Dec 8, 2020

s3select fixes and tests for trim and substring ceph/s3-tests#375

Open

the purpose fo this commit is to bypass boost::spirit rescan (calling…

dfb7532

… to bind-action more than once per the same text) Signed-off-by: gal salomon <gal.salomon@gmail.com>

albin-antony force-pushed the substr-fix branch from 319865c to 75b5105 Compare December 10, 2020 09:52

albin-antony mentioned this pull request Dec 10, 2020

the purpose fo this commit is to bypass boost::spirit rescan (calling… #41

Closed

s3select: fix substring

e720052

Align ceph s3select with aws. Signed-off-by: Albin Antony <aantony@redhat.com>

albin-antony force-pushed the substr-fix branch from 75b5105 to e720052 Compare December 14, 2020 13:43

albin-antony mentioned this pull request Dec 15, 2020

wip s3select: trim operator #36

Closed

albin-antony changed the title ~~s3select: fix substring~~ s3select: add trim operator , fix substring and bypass boost::spirit rescan Dec 15, 2020

galsalomon66 reviewed Dec 16, 2020

View reviewed changes

albin-antony and others added 2 commits December 16, 2020 13:59

add flags to CMAKE_CXX_FLAGS

f8c9550

Signed-off-by: Albin Antony <aantony@redhat.com>

wip s3select: trim operator

3e2328e

Signed-off-by: Albin Antony <aantony@redhat.com>

albin-antony force-pushed the substr-fix branch from c3e7174 to 3e2328e Compare December 16, 2020 08:29

galsalomon66 approved these changes Dec 26, 2020

View reviewed changes

galsalomon66 merged commit 0ca1e85 into ceph:master Dec 26, 2020

galsalomon66 mentioned this pull request Jan 3, 2021

S3select: replacing a "naked loop" with std::copy_n #42

Open


		substr_one_arg = (bsc::str_p("substring") >> '(' >> (arithmetic_expression >> bsc::str_p(",") >> arithmetic_expression) >> ')') [BOOST_BIND_ACTION(push_substr_one_arg)];

		substr_two_arg = (bsc::str_p("substring") >> '(' >> (arithmetic_expression >> bsc::str_p(",") >> arithmetic_expression >> bsc::str_p(",") >> arithmetic_expression) >> ')') [BOOST_BIND_ACTION(push_substr_two_arg)];


		substr_from = (bsc::str_p("substring") >> '(' >> (arithmetic_expression >> bsc::str_p("from") >> arithmetic_expression) >> ')') [BOOST_BIND_ACTION(push_substr_from)];

		substr_from_for = (bsc::str_p("substring") >> '(' >> (arithmetic_expression >> bsc::str_p("from") >> arithmetic_expression >> bsc::str_p("for") >> arithmetic_expression) >> ')') [BOOST_BIND_ACTION(push_substr_from_for)];

Conversation

albin-antony commented Dec 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galsalomon66 Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galsalomon66 commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albin-antony commented Dec 4, 2020

Uh oh!

galsalomon66 commented Dec 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galsalomon66 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

galsalomon66 Dec 4, 2020 •

edited

Loading

galsalomon66 commented Dec 4, 2020 •

edited

Loading