ARROW-5892 : [C++][Gandiva] Support function aliases #4835

pprudhvi · 2019-07-09T18:12:36Z

Access precompiled functions with different names.

praveenbingo

please also add a test from gandiva java..

praveenbingo · 2019-07-10T08:34:20Z

c_glib/gandiva-glib/native-function.cpp

  auto gandiva_native_function =
    ggandiva_native_function_get_raw(native_function);
-  auto gandiva_function_signature = gandiva_native_function->signature();
+  auto gandiva_function_signature = gandiva_native_function->signatures().back();


wont they missing the aliases now if we do this..

praveenbingo · 2019-07-10T08:36:14Z

cpp/src/gandiva/expression_registry.h

 class GANDIVA_EXPORT ExpressionRegistry {
 public:
-  using iterator = const NativeFunction*;
+  using nf_iterator = const NativeFunction*;


better names please..its hard to follow otherwise..

renamed nf_iterator to native_func_iterator_type and fs_iterator to func_sig_iterator_type.
thanks for reviewing

codecov-io · 2019-07-10T17:42:03Z

Codecov Report

Merging #4835 into master will increase coverage by 1.65%.
The diff coverage is 99.29%.

@@            Coverage Diff             @@
##           master    #4835      +/-   ##
==========================================
+ Coverage   87.43%   89.08%   +1.65%     
==========================================
  Files         997      720     -277     
  Lines      139841   100216   -39625     
  Branches     1418        0    -1418     
==========================================
- Hits       122273    89282   -32991     
+ Misses      17206    10934    -6272     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
cpp/src/gandiva/expression_registry.h	`100% <ø> (ø)`	⬆️
cpp/src/gandiva/function_registry_common.h	`100% <ø> (ø)`	⬆️
cpp/src/gandiva/function_registry_string.cc	`100% <100%> (ø)`	⬆️
cpp/src/gandiva/expression_registry.cc	`90.59% <100%> (+1.07%)`	⬆️
cpp/src/gandiva/function_registry_datetime.cc	`100% <100%> (ø)`	⬆️
cpp/src/gandiva/function_registry_test.cc	`100% <100%> (ø)`	⬆️
cpp/src/gandiva/function_registry.cc	`100% <100%> (ø)`	⬆️
cpp/src/gandiva/native_function.h	`100% <100%> (ø)`	⬆️
cpp/src/gandiva/function_registry_math_ops.cc	`100% <100%> (ø)`	⬆️
cpp/src/gandiva/function_registry_arithmetic.cc	`100% <100%> (ø)`	⬆️
... and 282 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 167bad1...727bbd0. Read the comment docs.

fsaintjacques · 2019-07-10T18:48:28Z

cpp/src/gandiva/native_function.h

        result_nullable_type_(result_nullable_type),
-        pc_name_(pc_name) {}
+        pc_name_(pc_name) {
+    aliases.insert(aliases.begin(), base_name);


Don't modify this, instead, make aliases a const-ref, and call signatures_.push_back(Func..(base_name, ...)) before the loop.

done. thanks for reviewing.

fsaintjacques · 2019-07-10T19:02:44Z

cpp/src/gandiva/function_registry_test.cc

  const NativeFunction* function = registry_.LookupSignature(add_i32_i32);
  EXPECT_NE(function, nullptr);
-  EXPECT_EQ(function->signature(), add_i32_i32);
+  EXPECT_TRUE(std::find(function->signatures().begin(), function->signatures().end(),


This should do the trick EXPECT_THAT(add_i32_i32, In(function->signatures()).

using Contains matcher from gmock. couldn't find In.

kou · 2019-07-11T00:18:52Z

I'll push a commit for GLib related part to this branch in a few days.

bkietz · 2019-07-11T12:16:44Z

cpp/src/gandiva/function_registry_common.h

-  NativeFunction(#NAME, DataTypeVector{TYPE(), TYPE()}, TYPE(), kResultNullIfNull, \
-                 ARROW_STRINGIFY(NAME##_##TYPE##_##TYPE))
+#define BINARY_SYMMETRIC_SAFE_NULL_IF_NULL(NAME, ALIASES, TYPE)          \
+  NativeFunction(#NAME, ALIASES, DataTypeVector{TYPE(), TYPE()}, TYPE(), \


Macros which take initializer_lists are tricky because the lack of a paren means that an initializer_list with more than one element is considered two tokens. For example:

#define VECTOR(T, elements) ::std::vector<T>(elements) VECTOR(int, {1, 2, 3}); // error: the macro's arguments are "int", " {1", " 2", " 3}"

If you add the container type explicitly then it should work to just enclose the initializer_list in parens:

Suggested change

NativeFunction(#NAME, ALIASES, DataTypeVector{TYPE(), TYPE()}, TYPE(), \

NativeFunction(#NAME,

std::vector<std::string> ALIASES, DataTypeVector{TYPE(), TYPE()}, TYPE(), \

Please add a function with at least two aliases (in a test would be acceptable) to demonstrate how the macro can be used with plural aliases.

changed. Thanks for reviewing.

To clarify: you don't need to wrap all initializer lists in parentheses, just those with more than a single element

Access precompiled functions with different names. Author: Prudhvi Porandla <prudhvi.porandla@icloud.com> Author: pprudhvi <30983614+pprudhvi@users.noreply.github.com> Closes apache#4835 from pprudhvi/func-aliases and squashes the following commits: 39bc74f <Prudhvi Porandla> enclose in parens only when required a5ad80e <Prudhvi Porandla> lint 8eb9cc1 <Prudhvi Porandla> do not pass initializer list through macros directly 430b97f <Prudhvi Porandla> refactor: make aliases const-ref, use gmock matcher 727bbd0 <pprudhvi> Merge branch 'master' into func-aliases 0610288 <Prudhvi Porandla> add base name at front. add get_all_signatures method in glib 0376231 <Prudhvi Porandla> add java test, rename iterator pointer names e0d02b1 <Prudhvi Porandla> use nullptr instead of NULLPTR 3540703 <Prudhvi Porandla> use native funciton's last func signature in glib ee2ef21 <Prudhvi Porandla> refactor: descriptive member names in func_registry 61c29a2 <Prudhvi Porandla> add aliases to truncate, divide, mod 090b6a7 <Prudhvi Porandla> support function aliases

This is follow-up for ARROW-5892: apache#4835

This is follow-up for ARROW-5892: #4835 Author: Sutou Kouhei <kou@clear-code.com> Closes #4874 from kou/glib-gandiva-function-aliases and squashes the following commits: 306d064 <Sutou Kouhei> Add symbol list for 1.0.0 5f173d6 <Sutou Kouhei> Add support for function aliases

Access precompiled functions with different names. Author: Prudhvi Porandla <prudhvi.porandla@icloud.com> Author: pprudhvi <30983614+pprudhvi@users.noreply.github.com> Closes #4835 from pprudhvi/func-aliases and squashes the following commits: 39bc74f <Prudhvi Porandla> enclose in parens only when required a5ad80e <Prudhvi Porandla> lint 8eb9cc1 <Prudhvi Porandla> do not pass initializer list through macros directly 430b97f <Prudhvi Porandla> refactor: make aliases const-ref, use gmock matcher 727bbd0 <pprudhvi> Merge branch 'master' into func-aliases 0610288 <Prudhvi Porandla> add base name at front. add get_all_signatures method in glib 0376231 <Prudhvi Porandla> add java test, rename iterator pointer names e0d02b1 <Prudhvi Porandla> use nullptr instead of NULLPTR 3540703 <Prudhvi Porandla> use native funciton's last func signature in glib ee2ef21 <Prudhvi Porandla> refactor: descriptive member names in func_registry 61c29a2 <Prudhvi Porandla> add aliases to truncate, divide, mod 090b6a7 <Prudhvi Porandla> support function aliases

This is follow-up for ARROW-5892: #4835 Author: Sutou Kouhei <kou@clear-code.com> Closes #4874 from kou/glib-gandiva-function-aliases and squashes the following commits: 306d064 <Sutou Kouhei> Add symbol list for 1.0.0 5f173d6 <Sutou Kouhei> Add support for function aliases

Access precompiled functions with different names. Author: Prudhvi Porandla <prudhvi.porandla@icloud.com> Author: pprudhvi <30983614+pprudhvi@users.noreply.github.com> Closes apache#4835 from pprudhvi/func-aliases and squashes the following commits: 39bc74f <Prudhvi Porandla> enclose in parens only when required a5ad80e <Prudhvi Porandla> lint 8eb9cc1 <Prudhvi Porandla> do not pass initializer list through macros directly 430b97f <Prudhvi Porandla> refactor: make aliases const-ref, use gmock matcher 727bbd0 <pprudhvi> Merge branch 'master' into func-aliases 0610288 <Prudhvi Porandla> add base name at front. add get_all_signatures method in glib 0376231 <Prudhvi Porandla> add java test, rename iterator pointer names e0d02b1 <Prudhvi Porandla> use nullptr instead of NULLPTR 3540703 <Prudhvi Porandla> use native funciton's last func signature in glib ee2ef21 <Prudhvi Porandla> refactor: descriptive member names in func_registry 61c29a2 <Prudhvi Porandla> add aliases to truncate, divide, mod 090b6a7 <Prudhvi Porandla> support function aliases

pprudhvi added 5 commits July 9, 2019 17:42

support function aliases

090b6a7

add aliases to truncate, divide, mod

61c29a2

refactor: descriptive member names in func_registry

ee2ef21

use native funciton's last func signature in glib

3540703

use nullptr instead of NULLPTR

e0d02b1

praveenbingo reviewed Jul 10, 2019

View reviewed changes

pprudhvi added 2 commits July 10, 2019 14:40

add java test, rename iterator pointer names

0376231

add base name at front. add get_all_signatures method in glib

0610288

pprudhvi closed this Jul 10, 2019

pprudhvi reopened this Jul 10, 2019

pprudhvi closed this Jul 10, 2019

pprudhvi reopened this Jul 10, 2019

pprudhvi closed this Jul 10, 2019

pprudhvi reopened this Jul 10, 2019

pprudhvi changed the title ~~ARROW-5892 : [WIP][C++][Gandiva] Support function aliases~~ ARROW-5892 : [C++][Gandiva] Support function aliases Jul 10, 2019

pprudhvi marked this pull request as ready for review July 10, 2019 12:20

Merge branch 'master' into func-aliases

727bbd0

fsaintjacques reviewed Jul 10, 2019

View reviewed changes

refactor: make aliases const-ref, use gmock matcher

430b97f

pprudhvi closed this Jul 11, 2019

pprudhvi reopened this Jul 11, 2019

bkietz requested changes Jul 11, 2019

View reviewed changes

pprudhvi added 3 commits July 11, 2019 23:43

do not pass initializer list through macros directly

8eb9cc1

lint

a5ad80e

enclose in parens only when required

39bc74f

pravindra approved these changes Jul 12, 2019

View reviewed changes

pravindra closed this in 48200c4 Jul 12, 2019

kou added a commit to kou/arrow that referenced this pull request Jul 13, 2019

[GLib][Gandiva] Add support for function aliases

5f173d6

This is follow-up for ARROW-5892: apache#4835

kou mentioned this pull request Jul 13, 2019

ARROW-5943: [GLib][Gandiva] Add support for function aliases #4874

Closed

pprudhvi deleted the func-aliases branch September 5, 2019 07:12

asfimport mentioned this pull request Aug 1, 2019

[C++][Gandiva] Support function aliases #22306

Closed

	NativeFunction(#NAME, ALIASES, DataTypeVector{TYPE(), TYPE()}, TYPE(), \
	NativeFunction(#NAME,
	std::vector<std::string> ALIASES, DataTypeVector{TYPE(), TYPE()}, TYPE(), \

ARROW-5892 : [C++][Gandiva] Support function aliases #4835

ARROW-5892 : [C++][Gandiva] Support function aliases #4835

Uh oh!

Conversation

pprudhvi commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

praveenbingo left a comment

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!

pprudhvi Jul 10, 2019 • 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!

codecov-io commented Jul 10, 2019

Codecov Report

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!

kou commented Jul 11, 2019

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pprudhvi commented Jul 9, 2019 •

edited

Loading

pprudhvi Jul 10, 2019 •

edited

Loading