ARROW-3845: [Gandiva] [GLib] Add GGandivaNode by shiro615 · Pull Request #3006 · apache/arrow

shiro615 · 2018-11-21T02:01:33Z

This PR adds Node classes to create GGandivaExpression with the specified GGandivaNode.

codecov-io · 2018-11-21T04:22:54Z

Codecov Report

Merging #3006 into master will increase coverage by 1.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3006      +/-   ##
==========================================
+ Coverage   86.65%   87.67%   +1.01%     
==========================================
  Files         491      424      -67     
  Lines       69513    64073    -5440     
==========================================
- Hits        60239    56173    -4066     
+ Misses       9182     7900    -1282     
+ Partials       92        0      -92

Impacted Files	Coverage Δ
python/pyarrow/public-api.pxi	`54.34% <0%> (-0.73%)`	⬇️
rust/src/record_batch.rs
go/arrow/array/table.go
rust/src/array.rs
go/arrow/math/uint64_amd64.go
go/arrow/internal/testing/tools/bool.go
go/arrow/array/bufferbuilder.go
go/arrow/internal/bitutil/bitutil.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/array/null.go
... and 65 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 7e6bf41...c166c8f. Read the comment docs.

Because it contains multiple classes.

shiro615

Thank you for your support.
I’ve read each commit for details. This looks good.

shiro615 · 2018-11-21T23:54:06Z

c_glib/gandiva-glib/node.cpp

+
+#define GGANDIVA_NODE_GET_PRIVATE(object)                       \
+  static_cast<GGandivaNodePrivate *>(                           \
+    ggandiva_node_get_instance_private(                         \


Is that because G_TYPE_INSTANCE_GET_PRIVATE has been deprecated since version 2.58?
I'll send follow-up pull requests to remove G_TYPE_INSTANCE_GET_PRIVATE in other classes.

Yes. Go ahead!

kou

Thanks!
It looks almost good. Can you check my comments?

kou · 2018-11-21T23:50:34Z

c_glib/doc/gandiva-glib/gandiva-glib-docs.xml

  </part>

+  <part id="tree-expression-builder">
+    <title>Tree Expression Builder</title>


How about "Expression Tree"? We hide tree expression builder in implementation. So it's better that we also hide it in document.

That makes sense. I've changed it.

kou · 2018-11-21T23:54:35Z

c_glib/gandiva-glib/expression.cpp

- * @input_fields: (element-type GArrowField): The input fields.
- * @output_field: A #GArrowField to be output.
+ * @node: The node in the expression tree.
+ * @field: A #GArrowField to be output.


Can you use root_node and result_field here?
Parameter name is important to describe parameter value.

I've renamed it.

kou · 2018-11-21T23:55:38Z

c_glib/gandiva-glib/expression.cpp

-                                             arrow_input_fields,
-                                             arrow_output_field);
+    gandiva::TreeExprBuilder::MakeExpression(gandiva_node,
+                                             arrow_field);


We need to keep a reference to GArrowField.
See f36ad38 how to implement it.

I've implemented it.

And I’ve implemented to keep a reference to GGandivaNode.
Please review again.

kou · 2018-11-21T23:57:31Z

c_glib/gandiva-glib/node.cpp

+
+#define GGANDIVA_NODE_GET_PRIVATE(object)                       \
+  static_cast<GGandivaNodePrivate *>(                           \
+    ggandiva_node_get_instance_private(                         \


Yes. Go ahead!

kou

We need more name changes.
Can you check my comments?

kou · 2018-11-22T07:20:26Z

c_glib/gandiva-glib/expression.cpp

-  PROP_EXPRESSION
+  PROP_EXPRESSION = 1,
+  PROP_NODE,
+  PROP_FIELD


root_node and return_field are better like the constructor parameter names.

I've changed it to use PROP_ROOT_NODE and PROP_RESULT_FIELD.

kou · 2018-11-22T07:21:07Z

c_glib/gandiva-glib/expression.cpp

- * @function: The function name in the expression.
- * @input_fields: (element-type GArrowField): The input fields.
- * @output_field: A #GArrowField to be output.
+ * @root_node: The node in the expression tree.


"The root node for the expression." may be better.

I've changed it.

kou · 2018-11-22T07:22:08Z

c_glib/gandiva-glib/expression.cpp

- * @input_fields: (element-type GArrowField): The input fields.
- * @output_field: A #GArrowField to be output.
+ * @root_node: The node in the expression tree.
+ * @result_field: A #GArrowField to be output.


"The name and type of returned value as #GArrowField." may be better.

I've changed it.

kou · 2018-11-22T07:22:59Z

c_glib/gandiva-glib/expression.cpp

 *
- * Returns: (transfer full): The expression tree with a root node,
+ * Returns: The expression tree with a root node,
 *   and a result field.


We can use simply "A newly created #GGandivaExpression" here.

I've changed it.

kou · 2018-11-22T07:24:41Z

c_glib/gandiva-glib/expression.cpp

-    arrow_input_fields.push_back(arrow_input_field);
-  }
-  auto arrow_output_field = garrow_field_get_raw(output_field);
+  auto gandiva_node = ggandiva_node_get_raw(root_node);


gandiva_root_node is better.

I've fixed it.

kou · 2018-11-22T07:24:48Z

c_glib/gandiva-glib/expression.cpp

-  }
-  auto arrow_output_field = garrow_field_get_raw(output_field);
+  auto gandiva_node = ggandiva_node_get_raw(root_node);
+  auto arrow_field = garrow_field_get_raw(result_field);


arrow_result_field is better.

I've fixed it.

kou · 2018-11-22T07:25:23Z

c_glib/gandiva-glib/expression.cpp

 GGandivaExpression *
-ggandiva_expression_new_raw(std::shared_ptr<gandiva::Expression> *gandiva_expression)
+ggandiva_expression_new_raw(std::shared_ptr<gandiva::Expression> *gandiva_expression,
+                            GGandivaNode *node,


root_node is better.

I've fixed it.

kou · 2018-11-22T07:25:37Z

c_glib/gandiva-glib/expression.cpp

-ggandiva_expression_new_raw(std::shared_ptr<gandiva::Expression> *gandiva_expression)
+ggandiva_expression_new_raw(std::shared_ptr<gandiva::Expression> *gandiva_expression,
+                            GGandivaNode *node,
+                            GArrowField *field)


result_field is better.

I've fixed it.

kou · 2018-11-22T07:26:01Z

c_glib/gandiva-glib/expression.h

-                                            GList *input_fields,
-                                            GArrowField *output_field);
+GGandivaExpression *ggandiva_expression_new(GGandivaNode *node,
+                                            GArrowField *field);


We need to change here too: root_node and return_field.

I've fixed it.

shiro615 · 2018-11-22T08:16:07Z

Oh... Thanks for your comments.
I'll try to address them to unify some names.

shiro615 · 2018-11-22T11:18:49Z

I think I've addressed them.
Please review again.

kou

+1
Thanks! I'll merge this.

shiro615 added 4 commits November 21, 2018 09:39

[GLib] Add Node classes for GGandiva

900fb33

Create GGandivaExpression with the specified GGandivaNode

cd1a8e1

Fix a typo

eb5ec7a

Add missing include

6f15863

shiro615 force-pushed the glib-add-ggandiva-node branch from 096b924 to 6f15863 Compare November 21, 2018 02:14

shiro615 changed the title ~~ARROW-3845: [Gandiva] [GLib] Add GGandivaNode~~ [WIP] ARROW-3845: [Gandiva] [GLib] Add GGandivaNode Nov 21, 2018

Fix red-gandiva test case

a4deb08

shiro615 force-pushed the glib-add-ggandiva-node branch from 25a1d99 to a4deb08 Compare November 21, 2018 03:37

shiro615 changed the title ~~[WIP] ARROW-3845: [Gandiva] [GLib] Add GGandivaNode~~ ARROW-3845: [Gandiva] [GLib] Add GGandivaNode Nov 21, 2018

kou added 5 commits November 21, 2018 14:39

Specify section ID explicitly

6f34b8d

Because it contains multiple classes.

Add "Gandiva" prefix

428e91e

Use get_instance_private() for GET_PRIVATE()

ec774ca

Keep reference to GArrowField to prevent freeing while using

f36ad38

Keep reference to argument to prevent freeing while using

84a5d67

shiro615 commented Nov 21, 2018

View reviewed changes

kou requested changes Nov 21, 2018

View reviewed changes

shiro615 added 6 commits November 22, 2018 09:15

Use 'Expression Tree' in document

c2bcc80

Change parameter name in ggandiva_expression_new()

f4c4420

Keep reference to GArrowField in GGandivaExpression

55fa3e5

Remove (transfer full) in ggandiva_expression_new()

62c3d8d

Keep reference to GGandivaNode in GGandivaExpression

1fcc223

Fix indents

f45ca8d

kou requested changes Nov 22, 2018

View reviewed changes

shiro615 added 4 commits November 22, 2018 18:57

Change property names to use root_node and result_field

75798e6

Change document in ggandiva_expression_new()

e06a323

Use each classes name not 'gandiva'

1903653

Fix argument names

c166c8f

kou approved these changes Nov 22, 2018

View reviewed changes

kou closed this in 2591454 Nov 22, 2018

shiro615 deleted the glib-add-ggandiva-node branch November 22, 2018 22:00

asfimport mentioned this pull request Nov 22, 2018

[Gandiva] [GLib] Add GGandivaNode #20349

Closed

Conversation

shiro615 commented Nov 21, 2018

Uh oh!

codecov-io commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shiro615 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!

kou 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!

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!

kou 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!

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!

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!

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!

shiro615 commented Nov 22, 2018

Uh oh!

shiro615 commented Nov 22, 2018

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

codecov-io commented Nov 21, 2018 •

edited

Loading