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

ronen-fr · 2020-12-14T14:18:12Z

... and fixing some extra copying

Signed-off-by: Ronen Friedman rfriedma@redhat.com

yuvalif · 2020-12-14T15:16:38Z

we dont have CI for this repo, but please make sure that unit tests are passing
please make sure that the submodule reference is updated in the ceph repo after the fix is merged
the "extra copy" issue exists in many other places in the code. to minimize the number of ceph submodule updates, could you please go through the rest of them?
at least the easy ones like const std::string& functions args and const auto& in loops?

include/s3select_oper.h

galsalomon66 · 2020-12-14T15:29:53Z

we dont have CI for this repo, but please make sure that unit tests are passing

please make sure that the submodule reference is updated in the ceph repo after the fix is merged

the "extra copy" issue exists in many other places in the code. to minimize the number of ceph submodule updates, could you please go through the rest of them?
at least the easy ones like const std::string& functions args and const auto& in loops?

there are a lot of tests, and it keeps growing.

unit-tests which cover the s3select-engine itself.
s3tests which validate integration with RGW.

the plan is to complete with s3select functionalities(as describe in AWS) , and to have first RGW-integration with parquet.
later, as separate effort, a performance "session".
one of the issues is to implement zero-copy (including string_view).

yuvalif · 2020-12-14T15:36:57Z

we dont have CI for this repo, but please make sure that unit tests are passing

please make sure that the submodule reference is updated in the ceph repo after the fix is merged

the "extra copy" issue exists in many other places in the code. to minimize the number of ceph submodule updates, could you please go through the rest of them?
at least the easy ones like const std::string& functions args and const auto& in loops?

there are a lot of tests, and it keeps growing.

unit-tests which cover the s3select-engine itself.

s3tests which validate integration with RGW.

the plan is to complete with s3select functionalities(as describe in AWS) , and to have first RGW-integration with parquet.
later, as separate effort, a performance "session".
one of the issues is to implement zero-copy (including string_view).

when making changes to this repo you can only run the unit tests.
after this PR is merged, and the submodule is updated in the ceph rtepo, you can also build the new ceph and run the s3select tests

galsalomon66 · 2020-12-14T15:46:11Z

we dont have CI for this repo, but please make sure that unit tests are passing

please make sure that the submodule reference is updated in the ceph repo after the fix is merged

the "extra copy" issue exists in many other places in the code. to minimize the number of ceph submodule updates, could you please go through the rest of them?
at least the easy ones like const std::string& functions args and const auto& in loops?

there are a lot of tests, and it keeps growing.

unit-tests which cover the s3select-engine itself.

s3tests which validate integration with RGW.

the plan is to complete with s3select functionalities(as describe in AWS) , and to have first RGW-integration with parquet.
later, as separate effort, a performance "session".
one of the issues is to implement zero-copy (including string_view).

when making changes to this repo you can only run the unit tests.
after this PR is merged, and the submodule is updated in the ceph rtepo, you can also build the new ceph and run the s3select tests

indeed.
with small correction, the PR merge and sub-module update is necessary for teuthology process , it's also possible to run private branch for s3-test in teuthology, before updating sub-module into ceph-repo.

it's always possible to test RGW integration (including valgrind) before merging the sub-module into ceph-repo.

include/s3select.h

include/s3select_oper.h

include/s3select.h

galsalomon66 · 2020-12-20T17:23:05Z

include/s3select_functions.h

@@ -1451,27 +1451,27 @@ bool base_statement::is_function()
  }
 }

-bool base_statement::is_aggregate_exist_in_expression(base_statement* e) //TODO obsolete ?
+bool base_statement::is_aggregate_exist_in_expression() const //TODO obsolete ?


indeed,
it better to change this interface, to avoid conflicts.
(this specific recursion function, should probably remove)

yuvalif · 2020-12-21T10:28:47Z

include/s3select.h

@@ -1191,7 +1188,6 @@ class csv_object : public base_s3object
  bool m_aggr_flow = false; //TODO once per query
  bool m_is_to_aggregate;
  bool m_skip_last_line;
-  size_t m_stream_length;


why did you remove that?

it appears not in use, (using other variable to mark end of-stream)

yuvalif · 2020-12-21T10:39:37Z

include/s3select_oper.h

-      {
-        l.__val.dbl = __op((double)l.__val.num, r.__val.dbl);
-        l.type = value_En_t::FLOAT;
+      if (l.type != r.type) {


what actually changed from here to the end of the function?
if its only indentation, please revert. this is difficult to review and does not add value here.

The problem was that the indentation was wrong (not just a style issue).
Line 824 started a block with ind' of 4 instead of 6, which affected the rest of the function.

as mentioned previously i'm using a utility which arrange those issues.
astyle --style=stroustrup --style=break --align-pointer=type --align-reference=type --pad-comma --add-braces --indent=spaces=2

it will be done on dedicated PR, since it create a lot of differences

ronen-fr · 2020-12-22T13:54:57Z

In the latest version I hope I have restored old formatting for all lines I did not change
for other reasons

galsalomon66 · 2020-12-22T23:31:42Z

include/s3select.h

@@ -1087,20 +1087,20 @@ void push_data_type::operator()(s3select* self, const char* a, const char* b) co

  if(cast_operator("int"))
  {
-    self->getAction()->dataTypeQ.push_back("int");
+    self->getAction()->dataTypeQ.emplace_back("int");


ok, it performs better.
should note, those functions (AST builder) used for building the AST and are not intensive.

galsalomon66 · 2020-12-23T09:26:35Z

there are currently several PR's on work, i will wait with those changes until one of them is merge.

galsalomon66 · 2020-12-31T07:54:28Z

include/s3select_functions.h

  s3select_functions* m_s3select_functions;
  variable m_result;

-  void _resolve_name()
+  void _resolve_name()  // todo: separate the object types used when constructing the __function vs when m_func_impl should be fixed


functions are created upon first use (first row) , kind of late-binding.
thus, it can not be const.
to avoid late-binding, function should be created upon building the AST, it means also to move semantic analysis to earlier stage.
this may happen on later phase, it's quite comprehensive change.

yuvalif · 2020-12-31T10:21:31Z

@ronen-fr can you please resolve conflicts?

... and fixing some extra copying Signed-off-by: Ronen Friedman <rfriedma@redhat.com>

const-correctness, overrides, extra copying, ... Signed-off-by: Ronen Friedman <rfriedma@redhat.com>

... and some undoing of formatting changes (and rebasing...) Signed-off-by: Ronen Friedman <rfriedma@redhat.com>

ronen-fr · 2020-12-31T14:58:03Z

@yuvalif rebased.
Also - I've undone more formatting changes.

…e /reolve_name/ of functions is now done upon semnatic phase before execution; semantic phase should be more the name-resolving(will be on later phase); [ #42 ] Signed-off-by: gal salomon <gal.salomon@gmail.com>

#46) * The semantic analysis for nested-calls of non-aggregate functions with aggregate function was wrong, this PR fix several issues. first it rejects illegal queries, second it executes correctly these use-cases. Signed-off-by: gal salomon <gal.salomon@gmail.com> * semantic analysis rejects illegal aggregation queries; it identifying wrong combinations of aggregation projection and non-aggregation projection Signed-off-by: gal salomon <gal.salomon@gmail.com> * marking subtree as runnable; changes to execution flow to support correct execution of nested&complex aggregation queries Signed-off-by: gal salomon <gal.salomon@gmail.com> * adding const qualifier to methods/variables; as part of this effort the /reolve_name/ of functions is now done upon semantic phase before execution; semantic phase should be more the name-resolving(will be on later phase); [ #42 ] Signed-off-by: gal salomon <gal.salomon@gmail.com>

yuvalif · 2021-01-03T08:57:59Z

@yuvalif rebased.
Also - I've undone more formatting changes.

changes of the last commit looks good.
but it seems like another rebase is needed :-(
lets merge after rebase is done, so code won't diverge again.

there are several active PR's in parallel (at least 3), those merged PR are blocker for some.
as for this PR, i already merged great part of it, manually.
if its possible to use the clang-utility, it could save effort.

galsalomon66 · 2021-01-03T11:58:28Z

include/s3select_functions.h

  {
-    _resolve_name();
+    const_cast<__function*>(this)->_resolve_name();


i removed the call to /resolve_name/

yuvalif · 2021-01-03T12:36:40Z

@yuvalif rebased.
Also - I've undone more formatting changes.

changes of the last commit looks good.
but it seems like another rebase is needed :-(
lets merge after rebase is done, so code won't diverge again.

there are several active PR's in parallel (at least 3), those merged PR are blocker for some.
as for this PR, i already merged great part of it, manually.

@galsalomon66 this is usually not recommended, it makes the rebase work much harder. ideally, you merge the urgent bug fix PRs, and then we rebase this PR and merge it

if its possible to use the clang-utility, it could save effort.

once this PR is merged I'll create a PR to add some "github actions" to this repo.
this could serve as an example on adding clang (or another) static analysis tool.

galsalomon66 · 2021-01-03T15:03:12Z

@yuvalif rebased.
Also - I've undone more formatting changes.

changes of the last commit looks good.
but it seems like another rebase is needed :-(
lets merge after rebase is done, so code won't diverge again.
there are several active PR's in parallel (at least 3), those merged PR are blocker for some.
as for this PR, i already merged great part of it, manually.

@galsalomon66 this is usually not recommended, it makes the rebase work much harder. ideally, you merge the urgent bug fix PRs, and then we rebase this PR and merge it

if its possible to use the clang-utility, it could save effort.

once this PR is merged I'll create a PR to add some "github actions" to this repo.
this could serve as an example on adding clang (or another) static analysis tool.

@yuvalif , lets done with it, sooner the better.
(this PR was not in plan, it must be reviewed, some parts were under discussions, the #39 "waited" for a month)

galsalomon66 · 2021-01-03T16:40:35Z

include/s3select.h

@@ -399,7 +400,7 @@ struct s3select : public bsc::grammar<s3select>
      }
    }

-    if (aggr_flow == true)
+    if (aggr_flow)


== true/false is style issue

'== true' is a bit over the line dividing 'style' and 'irregular and strange'...

And, BTW - the 'if' statement semantics is 'if the expression in parens evaluates to 'true' then do ...'.
No new semantic meaning is conveyed by the '== true'. It just adds to the reader's burden.

indeed.
there is nothing logical or practical about /x == true/
i probably wanted to remind myself its a boolean and not some number
will change that.

this style appears in our code-base.
[ find ./src -name '.cc' -o -name '.h' | xargs egrep '== false|== true' | grep -v s3select | wc ] ;

Signed-off-by: gal salomon <gal.salomon@gmail.com>

galsalomon66 · 2021-02-21T21:58:16Z

include/s3select_functions.h

@@ -474,42 +470,43 @@ struct _fn_to_int : public base_function

 struct _fn_to_float : public base_function


merge to master #64

* fixing text to float convertion (ronen-fr #42) Signed-off-by: gal salomon <gal.salomon@gmail.com> * fix integer conversion; add compile-warning flag Signed-off-by: gal salomon <gal.salomon@gmail.com>

* adding object to handle awscli response,first phase. adding warning flags; Signed-off-by: gal salomon <gal.salomon@gmail.com> * fixing text to float conversion (ronen-fr #42) (#64) Signed-off-by: gal salomon <gal.salomon@gmail.com> * fix integer conversion; add compile-warning flag Signed-off-by: gal salomon <gal.salomon@gmail.com> * fix stdin mode; fix compile warnings Signed-off-by: gal salomon <gal.salomon@gmail.com>

galsalomon66 · 2021-03-01T07:30:19Z

include/s3select_oper.h

  {
    {"null",reserve_word_en_t::S3S_NULL},{"NULL",reserve_word_en_t::S3S_NULL},
    {"nan",reserve_word_en_t::S3S_NAN},{"NaN",reserve_word_en_t::S3S_NAN}
  };

-  bool is_reserved_word(std::string & token)
+  static bool is_reserved_word(std::string & token)


for what purpose is the static qualifier ?

to indicate that this is actually a standalone function that does not need an instance of the object.
in theory, you could have taken the function declaration outside if the class.
if this is likely to change (e.g. in the future this function will look into data members of the class) than keep it without the "static" keyword.
if not, than either add "static" or take out of the class.

i'm aware for that role of static.
the is_reserved_word is not stand for itself (should be a member)

galsalomon66 · 2021-03-01T07:30:27Z

include/s3select_oper.h

  {
    return m_reserved_words.find(token) != m_reserved_words.end() ;
  }

-  reserve_word_en_t get_reserved_word(std::string & token)
+  static reserve_word_en_t get_reserved_word(std::string & token) // todo consider returning std::optional


for what purpose is the static qualifier ?

ronen-fr requested review from yuvalif and galsalomon66 December 14, 2020 14:18

galsalomon66 reviewed Dec 14, 2020

View reviewed changes

include/s3select_oper.h Show resolved Hide resolved

galsalomon66 approved these changes Dec 14, 2020

View reviewed changes

ronen-fr force-pushed the wip-ronenf-s3-sug branch from 6ddf0a2 to 60b11f6 Compare December 15, 2020 11:59

yuvalif reviewed Dec 15, 2020

View reviewed changes

include/s3select.h Outdated Show resolved Hide resolved

ronen-fr force-pushed the wip-ronenf-s3-sug branch from 60b11f6 to 86a344a Compare December 15, 2020 13:39

yuvalif approved these changes Dec 15, 2020

View reviewed changes

ronen-fr requested a review from galsalomon66 December 15, 2020 14:39

ronen-fr force-pushed the wip-ronenf-s3-sug branch from 8f00310 to d125159 Compare December 15, 2020 14:42

yuvalif reviewed Dec 16, 2020

View reviewed changes

include/s3select_oper.h Outdated Show resolved Hide resolved

galsalomon66 reviewed Dec 16, 2020

View reviewed changes

include/s3select.h Outdated Show resolved Hide resolved

galsalomon66 reviewed Dec 16, 2020

View reviewed changes

include/s3select.h Outdated Show resolved Hide resolved

ronen-fr force-pushed the wip-ronenf-s3-sug branch from d125159 to 3e7302f Compare December 16, 2020 16:12

ronen-fr requested review from yuvalif and galsalomon66 December 16, 2020 16:12

galsalomon66 reviewed Dec 20, 2020

View reviewed changes

yuvalif reviewed Dec 21, 2020

View reviewed changes

ronen-fr force-pushed the wip-ronenf-s3-sug branch from 3e7302f to 0e392f5 Compare December 22, 2020 13:48

ronen-fr requested a review from yuvalif December 22, 2020 13:55

galsalomon66 reviewed Dec 22, 2020

View reviewed changes

yuvalif approved these changes Dec 23, 2020

View reviewed changes

galsalomon66 mentioned this pull request Dec 28, 2020

replace virtual with override; adding const qualifier to variables,me… #44

Merged

galsalomon66 reviewed Dec 31, 2020

View reviewed changes

ronen-fr added 2 commits December 31, 2020 15:26

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

6c6d996

... and fixing some extra copying Signed-off-by: Ronen Friedman <rfriedma@redhat.com>

S3select: cleaning clang-tidy warnings

62320d6

const-correctness, overrides, extra copying, ... Signed-off-by: Ronen Friedman <rfriedma@redhat.com>

ronen-fr force-pushed the wip-ronenf-s3-sug branch 3 times, most recently from 24113a1 to 342b2b8 Compare December 31, 2020 14:42

s3select:clang-tidying of s3select_oper.h

9b6c158

... and some undoing of formatting changes (and rebasing...) Signed-off-by: Ronen Friedman <rfriedma@redhat.com>

ronen-fr force-pushed the wip-ronenf-s3-sug branch from 342b2b8 to 9b6c158 Compare December 31, 2020 14:52

galsalomon66 reviewed Jan 3, 2021

View reviewed changes

include/s3select_functions.h

{

_resolve_name();

const_cast<__function*>(this)->_resolve_name();

Copy link

galsalomon66 Jan 3, 2021

Choose a reason for hiding this comment

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

i removed the call to /resolve_name/

galsalomon66 reviewed Jan 3, 2021

View reviewed changes

galsalomon66 mentioned this pull request Jan 10, 2021

Wip ronenf s3 sug branch #48

Open

galsalomon66 added a commit that referenced this pull request Feb 21, 2021

fixing text to float convertion (ronen-fr #42)

d7680c6

Signed-off-by: gal salomon <gal.salomon@gmail.com>

galsalomon66 added a commit that referenced this pull request Feb 21, 2021

fixing text to float convertion (ronen-fr #42) (#64)

465c6ff

Signed-off-by: gal salomon <gal.salomon@gmail.com>

galsalomon66 reviewed Feb 21, 2021

View reviewed changes

galsalomon66 reviewed Mar 1, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ronen-fr commented Dec 14, 2020

yuvalif commented Dec 14, 2020

galsalomon66 commented Dec 14, 2020

yuvalif commented Dec 14, 2020

galsalomon66 commented Dec 14, 2020

galsalomon66 Dec 20, 2020

yuvalif Dec 21, 2020

galsalomon66 Dec 21, 2020

yuvalif Dec 21, 2020

ronen-fr Dec 22, 2020

galsalomon66 Dec 23, 2020

ronen-fr commented Dec 22, 2020

galsalomon66 Dec 22, 2020

galsalomon66 commented Dec 23, 2020

galsalomon66 Dec 31, 2020

yuvalif commented Dec 31, 2020

ronen-fr commented Dec 31, 2020

yuvalif commented Jan 3, 2021 •

edited by galsalomon66

galsalomon66 Jan 3, 2021

yuvalif commented Jan 3, 2021

galsalomon66 commented Jan 3, 2021

galsalomon66 Jan 3, 2021

ronen-fr Jan 4, 2021

galsalomon66 Jan 6, 2021

galsalomon66 Feb 21, 2021

galsalomon66 Mar 1, 2021

yuvalif Mar 1, 2021

galsalomon66 Apr 8, 2021

galsalomon66 Mar 1, 2021

		@@ -474,42 +470,43 @@ struct _fn_to_int : public base_function

		struct _fn_to_float : public base_function

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

Are you sure you want to change the base?

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

Conversation

ronen-fr commented Dec 14, 2020

yuvalif commented Dec 14, 2020

galsalomon66 commented Dec 14, 2020

yuvalif commented Dec 14, 2020

galsalomon66 commented Dec 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronen-fr commented Dec 22, 2020

Choose a reason for hiding this comment

galsalomon66 commented Dec 23, 2020

Choose a reason for hiding this comment

yuvalif commented Dec 31, 2020

ronen-fr commented Dec 31, 2020

yuvalif commented Jan 3, 2021 • edited by galsalomon66

Choose a reason for hiding this comment

yuvalif commented Jan 3, 2021

galsalomon66 commented Jan 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvalif commented Jan 3, 2021 •

edited by galsalomon66