ARROW-3966 [Java] JDBC Column Metadata in Arrow Field Metadata #3134

mikepigott · 2018-12-09T00:19:55Z

https://issues.apache.org/jira/browse/ARROW-3966

This change includes #3133, and supports a new configuration item called "Include Metadata." If true, metadata from the JDBC ResultSetMetaData object is pulled along to the Schema Field Metadata. For now, this includes:

Catalog Name
Table Name
Column Name
Column Type Name

…rrowConfig.

…sultSet resultSet, JdbcToArrowConfig config)

…n connection, String query, JdbcToArrowConfig config)

…eive JdbcToArrowConfig objects.

… field metadata.

Bring master up to date.

mikepigott · 2018-12-09T14:54:06Z

https://travis-ci.org/apache/arrow/jobs/465515849 - This appears to be a Travis / environment hiccup, and not a result of my change. Could someone restart this build?

wesm · 2018-12-09T17:16:48Z

You can trigger a fresh build by git commit --amend followed by git push -f. I'm restarting this build entry

wesm · 2018-12-14T16:59:54Z

@siddharthteotia @atuldambalkar can you review this?

Week Ending 12/15

12/30/18 Pulldown

Arrow Master 1/29/2019

atuldambalkar · 2019-01-30T10:20:00Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrowUtils.java

    Preconditions.checkNotNull(calendar, "Calendar object can't be null");

+    jdbcToArrowVectors(rs, root, new JdbcToArrowConfig(new RootAllocator(0), calendar));


I think, RootAllocator with argument 0, may not behave as expected. Have you tested this?

This is a little confusing. The config's allocator is used in the JdbcToArrow static member functions to construct a VectorSchemaRoot. At this point, the VectorSchemaRoot has already been constructed, so the allocator is an unused argument. That's why its allocation can be 0.

Yea, got it. Mainly for the backeward compatibilit for JdbcToArrowUtils.java class.

atuldambalkar · 2019-01-30T10:22:24Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrowUtils.java

          break;
      }
+
+      if (fieldType != null) {
+        fields.add(new Field(columnName, fieldType, null));


I like the use of FieldType object instead of creating Field object in each case stmt. I am fine with this.

emkornfield · 2019-02-02T07:35:34Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrow.java

+  /**
+   * For the given SQL query, execute and fetch the data from Relational DB and convert it to Arrow objects.
+   *
+   * @param connection Database connection to be used. This method will not close the passed connection object.


it might be a simpler contract to pass through the ResultSet object instead here?

That version exists on line 224.

This version can certainly be removed, but I was trying to preserve the pair of contracts the original author supplied. The two contracts were:

<Database + Query> + [Allocator | Calendar | Both | Neither]

+ [Allocator | Calendar | Both | Neither]

Since I'm adding a new contract of + , it made sense to me to also add <Database + Query> + .

emkornfield · 2019-02-02T07:36:51Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrow.java

@@ -89,7 +89,9 @@ public static VectorSchemaRoot sqlToArrow(Connection connection, String query, B
    Preconditions.checkArgument(query != null && query.length() > 0, "SQL query can not be null or empty");
    Preconditions.checkNotNull(allocator, "Memory allocator object can not be null");

-    return sqlToArrow(connection, query, allocator, Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT));
+    JdbcToArrowConfig config =
+            new JdbcToArrowConfig(allocator, Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT), false);


Would a utility method for getting the UTC calendar make sense?

Good call. Done.

emkornfield · 2019-02-02T07:44:17Z

java/adapter/jdbc/src/test/java/org/apache/arrow/adapter/jdbc/JdbcToArrowTestHelper.java

+      assertNotNull(metadata);
+      assertEquals(4, metadata.size());
+
+      assertEquals(rsmd.getCatalogName(i + 1), metadata.get(Constants.SQL_CATALOG_NAME_KEY));


might be cleaner if you start the loop at 1 instead of doing the addition in 4 places.

Yup, I agree. I had to use i - 1 when getting the field metadata from the VectorSchemaRoot, but I agree that's 3 fewer lines of code with an offset. Updated.

Arrow Master on 2/3

codecov-io · 2019-02-03T16:58:38Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3134      +/-   ##
==========================================
+ Coverage   87.82%   88.96%   +1.14%     
==========================================
  Files         666      441     -225     
  Lines       82371    53471   -28900     
  Branches     1069        0    -1069     
==========================================
- Hits        72341    47571   -24770     
+ Misses       9919     5900    -4019     
+ Partials      111        0     -111

Impacted Files	Coverage Δ
cpp/src/arrow/csv/reader.h	`0% <0%> (-100%)`	⬇️
cpp/src/arrow/array/builder_union.h	`0% <0%> (-100%)`	⬇️
cpp/src/arrow/array/builder_union.cc	`4.16% <0%> (-95.84%)`	⬇️
cpp/src/arrow/csv/reader.cc	`0.53% <0%> (-90.38%)`	⬇️
cpp/src/arrow/adapters/orc/adapter.cc	`0.26% <0%> (-73.04%)`	⬇️
cpp/src/arrow/csv/options.h	`66.66% <0%> (-33.34%)`	⬇️
cpp/src/parquet/column_scanner.cc	`59.09% <0%> (-22.73%)`	⬇️
cpp/src/arrow/vendored/xxhash/xxhash.c	`51.26% <0%> (-22.34%)`	⬇️
cpp/src/arrow/memory_pool.cc	`68.8% <0%> (-16.52%)`	⬇️
cpp/src/arrow/io/interfaces.cc	`40% <0%> (-15%)`	⬇️
... and 273 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 4004b72...02f2f34. Read the comment docs.

…ot locale.

mikepigott · 2019-02-03T17:17:43Z

Thanks for the code review! I believe I've responded to all of the feedback.

…offsets in code.

praveenbingo

sorry for the late review..

some of them are minor style comments..i am ok if you ignore/address in another PR..

praveenbingo · 2019-01-29T03:25:42Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrowConfig.java

+   * @return This instance of the <code>JdbcToArrowConfig</code>, for chaining.
+   * @exception NullPointerExeption if <code>calendar</code> is <code>null</code>.
+   */
+  public JdbcToArrowConfig setCalendar(Calendar calendar) {


minor style nit (please ignore if you disagree) i would prefer creating a builder, so that the POJO itself is immutable.

older comment..please ignore..

praveenbingo · 2019-01-29T03:26:17Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrowConfig.java

+   *
+   * @return Whether this configuration is valid.
+   */
+  public boolean isValid() {


using a builder would ensure that this object is always valid..and we can validate inside the builder.

older comment..please ignore..

praveenbingo · 2019-02-04T05:53:40Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrowConfig.java

+   *
+   * @return <code>true</code> to include field metadata, <code>false</code> to exclude it.
+   */
+  public boolean getIncludeMetadata() {


minor nit (please ignore if you dont agree :))

should this be named "shouldIncludeMetadata"

Good point; will fix tonight.

praveenbingo · 2019-02-04T05:56:03Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrowConfigBuilder.java

@@ -98,6 +137,6 @@ public JdbcToArrowConfigBuilder setCalendar(Calendar calendar) {
   * @throws NullPointerException if either the allocator or calendar was not set.
   */
  public JdbcToArrowConfig build() {
-    return new JdbcToArrowConfig(allocator, calendar);
+    return new JdbcToArrowConfig(allocator, calendar, includeMetadata);


(minor nit - again ignore if you dont agree)
can we default the include metadata in the builder so that only the clients that need it will override..

That makes sense. Will change tonight.

Hang on, I'm confused - includeMetadata is initialized to false in the builder. This is the behavior you asked for, right? Only people who call setIncludeMetadata(true) will have the metadata generated.

Ah I saw the ctors using the false flag in other places and was misled..this looks ok.

praveenbingo · 2019-02-04T05:58:25Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrowUtils.java

+
+      final Map<String, String> metadata;
+      if (config.getIncludeMetadata()) {
+        metadata = new HashMap<String, String>();


can use the <> operator to avoid the mention of types in RHS again.. https://www.javaworld.com/article/2074080/core-java/core-java-jdk-7-the-diamond-operator.html

Good point; will fix tonight.

praveenbingo · 2019-02-04T05:59:05Z

java/adapter/jdbc/src/main/java/org/apache/arrow/adapter/jdbc/JdbcToArrowUtils.java

+        metadata.put(Constants.SQL_TYPE_KEY, rsmd.getColumnTypeName(i));
+
+      } else {
+        metadata = null;


(minor nit)
do this in the initialization itself?..avoids the else block..

I can do that, but then I can't make the metadata variable final. I've worked on teams in the past where the best practice was to mark things final as often as possible; is that the same with Arrow?

mikepigott · 2019-02-05T02:15:10Z

Thanks for the feedback! I believe I've responded to all of it.

praveenbingo

+1. Looks good. Thanks @mikepigott

@wesm @xhochy - please help merging this..

xhochy · 2019-02-05T09:38:52Z

@mikepigott Can you rebase? Then I would merge this.

mikepigott · 2019-02-05T13:45:24Z

Sure thing; I'll merge master when I get home tonight. Thanks!

Arrow Master Through 2/5

mikepigott · 2019-02-06T01:50:19Z

Merged. Thanks for the review!

mikepigott · 2019-02-06T02:12:29Z

I noticed in the merge that one part of #3066 was lost in the shuffle, and I just added it back in. The calendar's time zone is used to create Timestamp types, even though it is not needed. I'm not sure why it was lost, but I just added it back.

xhochy

+1, LGTM

xhochy · 2019-02-06T17:11:09Z

Thanks!

https://issues.apache.org/jira/browse/ARROW-3966 This change includes #3133, and supports a new configuration item called "Include Metadata." If true, metadata from the JDBC ResultSetMetaData object is pulled along to the Schema Field Metadata. For now, this includes: * Catalog Name * Table Name * Column Name * Column Type Name Author: Mike Pigott <mpigott@gmail.com> Author: Michael Pigott <mikepigott@users.noreply.github.com> Closes #3134 from mikepigott/jdbc-column-metadata and squashes the following commits: 02f2f34 <Mike Pigott> ARROW-3966: Picking up lost change to support null calendars. 7049c36 <Mike Pigott> Merge branch 'master' into jdbc-column-metadata e9a9b2b <Michael Pigott> Merge pull request #6 from apache/master 65741a9 <Mike Pigott> ARROW-3966: Code review feedback cc6cc88 <Mike Pigott> ARROW-3966: Using a 1:N loop instead of a 0:N-1 loop for fewer index offsets in code. cfb2ba6 <Mike Pigott> ARROW-3966: Using a helper method for building a UTC calendar with root locale. 2928513 <Mike Pigott> ARROW-3966: Moving the metadata flag assignment into the builder. 69022c2 <Mike Pigott> ARROW-3966: Fixing merge. 4a6de86 <Mike Pigott> Merge branch 'master' into jdbc-column-metadata 509a1cc <Michael Pigott> Merge pull request #5 from apache/master 789c8c8 <Michael Pigott> Merge pull request #4 from apache/master e5b19ee <Michael Pigott> Merge pull request #3 from apache/master 3b17c29 <Michael Pigott> Merge pull request #2 from apache/master d847ebc <Mike Pigott> Fixing file location 1ceac9e <Mike Pigott> Merge branch 'master' into jdbc-column-metadata 881c6c8 <Michael Pigott> Merge pull request #1 from apache/master 03091a8 <Mike Pigott> Unit tests for including result set metadata. 72d64cc <Mike Pigott> Affirming the field metadata is empty when the configuration excludes field metadata. 7b4527c <Mike Pigott> Test for the include-metadata flag in the configuration. 7e9ce37 <Mike Pigott> Merge branch 'jdbc-to-arrow-config' into jdbc-column-metadata bb3165b <Mike Pigott> Updating the function calls to use the JdbcToArrowConfig versions. a6fb1be <Mike Pigott> Fixing function call 5bfd6a2 <Mike Pigott> Merge branch 'jdbc-to-arrow-config' into jdbc-column-metadata 68c91e7 <Mike Pigott> Modifying the jdbcToArrowSchema and jdbcToArrowVectors methods to receive JdbcToArrowConfig objects. b5b0cb1 <Mike Pigott> Merge branch 'jdbc-to-arrow-config' into jdbc-column-metadata 8d6cf00 <Mike Pigott> Documentation for public static VectorSchemaRoot sqlToArrow(Connection connection, String query, JdbcToArrowConfig config) 4f1260c <Mike Pigott> Adding documentation for public static VectorSchemaRoot sqlToArrow(ResultSet resultSet, JdbcToArrowConfig config) e34a9e7 <Mike Pigott> Fixing formatting. fe097c8 <Mike Pigott> Merge branch 'jdbc-to-arrow-config' into jdbc-column-metadata df632e3 <Mike Pigott> Updating the SQL tests to include JdbcToArrowConfig versions. b270044 <Mike Pigott> Updated validaton & documentation, and unit tests for the new JdbcToArrowConfig. da77cbe <Mike Pigott> Creating a configuration class for the JDBC-to-Arrow converter. a78c770 <Mike Pigott> Updating Javadocs. 523387f <Mike Pigott> Updating the API to support an optional 'includeMetadata' field. 5af1b5b <Mike Pigott> Separating out the field-type creation from the field creation.

https://issues.apache.org/jira/browse/ARROW-4142 This adds support for reading JDBC arrays and converting them to Arrow ListVectors. JDBC does not provide a great way to get the array type; there is a ResultSet object for walking the array, but its ResultSetMetaData may not contain the right value type (H2, for example, returns a JDBC type of NULL). This is based on #3134, which includes ARROW-3965 and ARROW-3966. I found Arrow arrays to be very confusing, and I am not sure if I am using them correctly here. One thing I noticed was if I added a null array to a ListVector of VarCharVectors, the next value in the VarCharVector would be empty. I would appreciate any help on why! The ListVector unit tests weren't very helpful. For all other cases, this code seems to work. I look forward to your review! Author: Mike Pigott <mpigott@gmail.com> Closes #3294 from mikepigott/jdbc-array-field and squashes the following commits: 66376dd <Mike Pigott> Support for reading Array records to ListVector from JDBC

https://issues.apache.org/jira/browse/ARROW-4142 This adds support for reading JDBC arrays and converting them to Arrow ListVectors. JDBC does not provide a great way to get the array type; there is a ResultSet object for walking the array, but its ResultSetMetaData may not contain the right value type (H2, for example, returns a JDBC type of NULL). This is based on apache#3134, which includes ARROW-3965 and ARROW-3966. I found Arrow arrays to be very confusing, and I am not sure if I am using them correctly here. One thing I noticed was if I added a null array to a ListVector of VarCharVectors, the next value in the VarCharVector would be empty. I would appreciate any help on why! The ListVector unit tests weren't very helpful. For all other cases, this code seems to work. I look forward to your review! Author: Mike Pigott <mpigott@gmail.com> Closes apache#3294 from mikepigott/jdbc-array-field and squashes the following commits: 66376dd <Mike Pigott> Support for reading Array records to ListVector from JDBC

Mike Pigott and others added 22 commits December 4, 2018 20:24

Separating out the field-type creation from the field creation.

5af1b5b

Updating the API to support an optional 'includeMetadata' field.

523387f

Updating Javadocs.

a78c770

Creating a configuration class for the JDBC-to-Arrow converter.

da77cbe

Updated validaton & documentation, and unit tests for the new JdbcToA…

b270044

…rrowConfig.

Updating the SQL tests to include JdbcToArrowConfig versions.

df632e3

Merge branch 'jdbc-to-arrow-config' into jdbc-column-metadata

fe097c8

Fixing formatting.

e34a9e7

Adding documentation for public static VectorSchemaRoot sqlToArrow(Re…

4f1260c

…sultSet resultSet, JdbcToArrowConfig config)

Documentation for public static VectorSchemaRoot sqlToArrow(Connectio…

8d6cf00

…n connection, String query, JdbcToArrowConfig config)

Merge branch 'jdbc-to-arrow-config' into jdbc-column-metadata

b5b0cb1

Modifying the jdbcToArrowSchema and jdbcToArrowVectors methods to rec…

68c91e7

…eive JdbcToArrowConfig objects.

Merge branch 'jdbc-to-arrow-config' into jdbc-column-metadata

5bfd6a2

Fixing function call

a6fb1be

Updating the function calls to use the JdbcToArrowConfig versions.

bb3165b

Merge branch 'jdbc-to-arrow-config' into jdbc-column-metadata

7e9ce37

Test for the include-metadata flag in the configuration.

7b4527c

Affirming the field metadata is empty when the configuration excludes…

72d64cc

… field metadata.

Unit tests for including result set metadata.

03091a8

Merge pull request #1 from apache/master

881c6c8

Bring master up to date.

Merge branch 'master' into jdbc-column-metadata

1ceac9e

Fixing file location

d847ebc

mikepigott added 2 commits December 15, 2018 15:46

Merge pull request #2 from apache/master

3b17c29

Week Ending 12/15

Merge pull request #3 from apache/master

e5b19ee

12/30/18 Pulldown

mikepigott mentioned this pull request Jan 1, 2019

ARROW-4142: [Java] JDBC Array -> Arrow ListVector #3294

Closed

Merge pull request #4 from apache/master

789c8c8

Arrow Master 1/29/2019

atuldambalkar reviewed Jan 30, 2019

View reviewed changes

emkornfield reviewed Feb 2, 2019

View reviewed changes

mikepigott and others added 4 commits February 3, 2019 10:52

Merge pull request #5 from apache/master

509a1cc

Arrow Master on 2/3

Merge branch 'master' into jdbc-column-metadata

4a6de86

ARROW-3966: Fixing merge.

69022c2

ARROW-3966: Moving the metadata flag assignment into the builder.

2928513

ARROW-3966: Using a helper method for building a UTC calendar with ro…

cfb2ba6

…ot locale.

ARROW-3966: Using a 1:N loop instead of a 0:N-1 loop for fewer index …

cc6cc88

…offsets in code.

praveenbingo reviewed Feb 4, 2019

View reviewed changes

ARROW-3966: Code review feedback

65741a9

praveenbingo approved these changes Feb 5, 2019

View reviewed changes

mikepigott and others added 2 commits February 5, 2019 20:44

Merge pull request #6 from apache/master

e9a9b2b

Arrow Master Through 2/5

Merge branch 'master' into jdbc-column-metadata

7049c36

ARROW-3966: Picking up lost change to support null calendars.

02f2f34

xhochy approved these changes Feb 6, 2019

View reviewed changes

xhochy closed this in 54182b2 Feb 6, 2019

asfimport mentioned this pull request Feb 6, 2019

[Java] JDBC-to-Arrow Conversion: JDBC Metadata in Schema Fields #20569

Closed

		Preconditions.checkNotNull(calendar, "Calendar object can't be null");

		jdbcToArrowVectors(rs, root, new JdbcToArrowConfig(new RootAllocator(0), calendar));

ARROW-3966 [Java] JDBC Column Metadata in Arrow Field Metadata #3134

ARROW-3966 [Java] JDBC Column Metadata in Arrow Field Metadata #3134

Conversation

mikepigott commented Dec 9, 2018

mikepigott commented Dec 9, 2018

wesm commented Dec 9, 2018

wesm commented Dec 14, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 3, 2019 • edited Loading

Codecov Report

mikepigott commented Feb 3, 2019

praveenbingo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepigott commented Feb 5, 2019

praveenbingo left a comment

Choose a reason for hiding this comment

xhochy commented Feb 5, 2019

mikepigott commented Feb 5, 2019

mikepigott commented Feb 6, 2019

mikepigott commented Feb 6, 2019

xhochy left a comment

Choose a reason for hiding this comment

xhochy commented Feb 6, 2019

codecov-io commented Feb 3, 2019 •

edited

Loading