SNOW-1374896 unify structured types string representation #1882

sfc-gh-mkubik · 2024-09-02T07:49:50Z

Overview

Build string representations of Snowflake structured types recursively to reuse existing converters design for specific logical types (e.g. timestamps/binary)

Code replaces the existing structured types converters implementation that was running the native getObject method with a solution that utilises reading a field vectors within the structured type and running a proper converter on each nested type. Changes are made to Array, Map and Struct converters, helper methods are added to ArrowVectorConverter interface and new ArrowStringRepresentationBuilder classes that abstract away the logic of actually building a string object out of the arrow structured type.

Follow ups:

pretty print - currently the builders don't add new lines or tabs to the string representation as I think it makes the code more readable but the downside is that it causes some divergence between ARROW and JSON (which is pretty printed). Potential solution is adding some setting that enables pretty print and converting it once the string is built (to avoid passing the depth to recursive toString calls)
recursive call of ARROW converters returns null while for JSON there's undefined which also is some kind of divergence but not necessarily something to fix as ARROW's null sounds more reasonable

example for SELECT [12, 10, 5, NULL]::ARRAY(DOUBLE)

JSON                                                         | ARROW
[                                                            | [12.0,10.0,5.0,null]
  1.200000000000000e+01,
  1.000000000000000e+01,
  5.000000000000000e+00,
  undefined
]

Pre-review self checklist

PR branch is updated with all the changes from master branch
The code is correctly formatted (run mvn -P check-style validate)
New public API is not unnecessary exposed (run mvn verify and inspect target/japicmp/japicmp.html)
The pull request name is prefixed with SNOW-XXXX:
Code is in compliance with internal logging requirements

Add helper ArrowStringRepresentationBuilders that take care of converting recursive toString results into a valid json, taking logical type into accunt. Extract fetching logical type from field metadata to a separate static function, change boolean string representations to lowercase, add tests.

sfc-gh-astachowski

It might be worth adding custom to string for vectors, in case vector ever accepts types other than int and float.

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java

sfc-gh-mkubik · 2024-09-02T11:53:01Z

expected:<[FALSE]> but was:<[false]>

Seems I haven't change all ocurrences of upper case booleans in tests, will fix in next commit

src/test/java/net/snowflake/client/jdbc/structuredtypes/StructuredTypesGetStringBaseIT.java

.../net/snowflake/client/core/arrow/tostringhelpers/ArrowObjectStringRepresentationBuilder.java

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java

Move prefix and suffix cofiguration to the constructor of base builder, remove unnecessary comments, extract shouldQuote check to a super method, make valueType a constructor parameter for Array toString builder, fix tests failing due to the lowercase booleans

Add helper ArrowStringRepresentationBuilders that take care of converting recursive toString results into a valid json, taking logical type into accunt. Extract fetching logical type from field metadata to a separate static function, change boolean string representations to lowercase, add tests.

Move prefix and suffix cofiguration to the constructor of base builder, remove unnecessary comments, extract shouldQuote check to a super method, make valueType a constructor parameter for Array toString builder, fix tests failing due to the lowercase booleans

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java

.../net/snowflake/client/core/arrow/tostringhelpers/ArrowObjectStringRepresentationBuilder.java

Comments to be addressed

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java

sfc-gh-pmotacki · 2024-09-05T06:24:38Z

src/main/java/net/snowflake/client/core/arrow/ArrayConverter.java

@@ -21,6 +24,25 @@ public Object toObject(int index) throws SFException {

  @Override
  public String toString(int index) throws SFException {
-    return vector.getObject(index).toString();
+    FieldVector vectorUnpacked = vector.getChildrenFromFields().get(0);


Are we sure that there must be at least one child inside? Is get(0) safe? Is it checked somewhere before?

It's a given for any ListVector

sfc-gh-pmotacki · 2024-09-05T07:16:03Z

src/main/java/net/snowflake/client/core/arrow/MapConverter.java

+    FieldVector vectorUnpacked = vector.getChildrenFromFields().get(0);
+
+    FieldVector keys = vectorUnpacked.getChildrenFromFields().get(0);
+    FieldVector values = vectorUnpacked.getChildrenFromFields().get(1);


Should we verify here that the children set contains key-children and value-children?

I belive this should always work for map vector, but I'll verify for empty one

Ok, so there might exist an object of MapVector class that does not have these children, but it seems to be a very weird case. We could either try and verify that it won't happen here (which is probably the case), or simply add a check just to be extra safe.

After verification, this shouldn't be empty if used properly, so we are good

According to MapVector docs it seems that we're good as we're checking isSet and

The MapVector is nullable, but if a map is set at a given index, there must be an entry.

sfc-gh-pmotacki · 2024-09-05T07:20:36Z

src/main/java/net/snowflake/client/core/arrow/MapConverter.java

+    }
+
+    for (int i = vector.getElementStartIndex(index); i < vector.getElementEndIndex(index); i++) {
+      builder.appendKeyValue(


What about keyLogicalType? I know that it could only String but it must be changed in future because database could return also Integers keys.

It's used only to make the decision on whether the value should be quoted or not. The output string is JSON-like so the key is always quoted even for Integers

sfc-gh-pmotacki · 2024-09-05T07:35:03Z

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java

+  }
+
+  public ArrowStringRepresentationBuilderBase appendValue(String value) {
+    addCommaIfNeeded();


Did you weigh the pros and cons of using StringJoiner?

I only considered two extreme cases of building the string manually (chosen) and using some abstraction like JSONObject (rejected) but didn't consider StringJointer which is an option in between so I'll also take a look at it

sfc-gh-pmotacki · 2024-09-05T07:35:42Z

src/test/java/net/snowflake/client/jdbc/BaseWiremockTest.java

+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+
+public abstract class BaseWiremockTest {


This class is out of scope, true?

Yeees, it appeared here after rebase for some reason but it's a change that should already be merged I believe

sfc-gh-pmotacki · 2024-09-05T07:38:05Z

.../snowflake/client/jdbc/structuredtypes/StructuredTypesGetStringArrowJsonCompatibilityIT.java

+
+@RunWith(Parameterized.class)
+@Category(TestCategoryResultSet.class)
+public class StructuredTypesGetStringArrowJsonCompatibilityIT


Can we add test for AllTypesClass structure?

Sure, will add

…ationBuilder

…lass

sfc-gh-astachowski and others added 15 commits August 22, 2024 09:01

Refactor init converters for reuse in other code

d8faeba

Formatting

8499a52

Removed star imports

982b13e

Documentation style fix

c82e155

Moved init converters to an already public class

7a8382b

Moved init converters to an appropriate interface

7aa57d1

Merge branch 'master' into init-converters-refactor

7246575

Changed the version of japicmp to include a needed bugfix

48c2435

Refactored unnecessary static modifiers

6142094

Reformat

a9ca5e6

Update japicmp to the newest version

e1c69e6

Merge branch 'master' into init-converters-refactor

d9701bc

fix formatting

777c7d0

mark new classes as internal

5ee4904

sfc-gh-mkubik requested a review from a team as a code owner September 2, 2024 07:49

fix failing boolean test, remove integer from quotable types

655df20

sfc-gh-astachowski reviewed Sep 2, 2024

View reviewed changes

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java Outdated Show resolved Hide resolved

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java Outdated Show resolved Hide resolved

Base automatically changed from init-converters-refactor to master September 2, 2024 14:01

sfc-gh-pbulawa requested changes Sep 3, 2024

View reviewed changes

sfc-gh-mkubik and others added 9 commits September 4, 2024 13:06

Address review comments

4d2eb63

Move prefix and suffix cofiguration to the constructor of base builder, remove unnecessary comments, extract shouldQuote check to a super method, make valueType a constructor parameter for Array toString builder, fix tests failing due to the lowercase booleans

Refactor init converters for reuse in other code

3a6dd28

Formatting

7668307

Removed star imports

9d41eb5

Documentation style fix

0c72b4b

Moved init converters to an already public class

9c325b9

fix formatting

ac7eb94

mark new classes as internal

6c6df45

sfc-gh-mkubik added 3 commits September 4, 2024 14:22

fix failing boolean test, remove integer from quotable types

c74f967

Address review comments

11be102

Move prefix and suffix cofiguration to the constructor of base builder, remove unnecessary comments, extract shouldQuote check to a super method, make valueType a constructor parameter for Array toString builder, fix tests failing due to the lowercase booleans

Resolve merge conflicts

277ffab

sfc-gh-pbulawa previously approved these changes Sep 4, 2024

View reviewed changes

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java Outdated Show resolved Hide resolved

.../net/snowflake/client/core/arrow/tostringhelpers/ArrowObjectStringRepresentationBuilder.java Outdated Show resolved Hide resolved

sfc-gh-pbulawa self-requested a review September 4, 2024 13:03

remove unnecessary toString override from child builder classes

67f3668

sfc-gh-astachowski reviewed Sep 4, 2024

View reviewed changes

...a/net/snowflake/client/core/arrow/tostringhelpers/ArrowArrayStringRepresentationBuilder.java Outdated Show resolved Hide resolved

remove unnecessary shouldQuote call

9a2c931

sfc-gh-pmotacki reviewed Sep 5, 2024

View reviewed changes

use StringJoiner instead of StringBuilder inside ArrowStringRepresent…

2f96b68

…ationBuilder

sfc-gh-pbulawa approved these changes Sep 11, 2024

View reviewed changes

sfc-gh-astachowski approved these changes Sep 13, 2024

View reviewed changes

sfc-gh-mkubik added 3 commits September 20, 2024 11:43

resolve merge conflicts

4f2bb5c

Fix usages of ArrowVectorConverter utils after moving to a separate c…

9887f74

…lass

fix formatting

542180d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1374896 unify structured types string representation #1882

SNOW-1374896 unify structured types string representation #1882

sfc-gh-mkubik commented Sep 2, 2024 •

edited

Loading

sfc-gh-astachowski left a comment

sfc-gh-mkubik commented Sep 2, 2024

sfc-gh-pmotacki Sep 5, 2024

sfc-gh-astachowski Sep 5, 2024

sfc-gh-pmotacki Sep 5, 2024

sfc-gh-astachowski Sep 5, 2024

sfc-gh-astachowski Sep 5, 2024

sfc-gh-astachowski Sep 5, 2024

sfc-gh-mkubik Sep 5, 2024

sfc-gh-pmotacki Sep 5, 2024

sfc-gh-mkubik Sep 5, 2024

sfc-gh-pmotacki Sep 5, 2024

sfc-gh-mkubik Sep 5, 2024

sfc-gh-pmotacki Sep 5, 2024

sfc-gh-mkubik Sep 5, 2024

sfc-gh-pmotacki Sep 5, 2024

sfc-gh-mkubik Sep 5, 2024

SNOW-1374896 unify structured types string representation #1882

Are you sure you want to change the base?

SNOW-1374896 unify structured types string representation #1882

Conversation

sfc-gh-mkubik commented Sep 2, 2024 • edited Loading

Overview

Pre-review self checklist

sfc-gh-astachowski left a comment

Choose a reason for hiding this comment

sfc-gh-mkubik commented Sep 2, 2024

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

sfc-gh-mkubik commented Sep 2, 2024 •

edited

Loading