[CALCITE-5964] Add test case checking that connection.getMetaData().getTables() handles responses with different signature shapes. #227

olivrlee · 2023-09-11T17:33:34Z

Also adding in Type.CLASS to ConnectionProperty.java and the associated getters.

…etTables() handles responses with different signature shapes. Also adding in Type.CLASS to ConnectionProperty.java and the associated getters.

Add in rows fix lint

wnob · 2023-09-12T21:52:54Z

core/src/main/java/org/apache/calcite/avatica/ConnectionConfigImpl.java

+
+    /** Returns the class value of this property. Throws if not set and no
+     * default. */
+    public Class<?> getClazz(Class<?> clazz, Class<?> defaultValue) {


nit: Java convention uses clazz because class is a keyword. getClass is not a keyword. Just call this getClass. Same goes for all other names that contain the word "class" but are not keywords.

There's a small chance for confusion with Object.getClass(). So, I would recommend calling it getType.

wnob · 2023-09-12T21:59:17Z

core/src/main/java/org/apache/calcite/avatica/ConnectionConfigImpl.java

@@ -434,6 +447,30 @@ public T apply(ConnectionProperty connectionProperty, String s) {
      }
    };
  }
+
+  public static <T> Converter<T> clazzConverter(final T clazz,


T can be narrowed to Class<T>, since there's only 1 callsite for this function and it will always pass in a Class<X> for some X. That would also make the variable name less confusing.

I suppose that would make this a Converter<Class<?>>. You may want to add another type variable for defaultClass.

wnob · 2023-09-12T22:28:20Z

core/src/main/java/org/apache/calcite/avatica/MetaImpl.java

@@ -388,6 +388,32 @@ public MetaColumn(
    public String getName() {
      return columnName;
    }
+    public static String[] getColumnNames() {


Might as well link to some documentation: https://docs.oracle.com/en/java/javase/20/docs/api/java.sql/java/sql/DatabaseMetaData.html#getColumns(java.lang.String,java.lang.String,java.lang.String,java.lang.String)

Also add a comment explaining why anybody might want to override this.

It's static so it can't be overridden, the reader thinks. The fact that the caller is using reflection to find and invoke it could definitely do with some commentary, if that is indeed the best way for the caller to get this information.

wnob · 2023-09-12T22:29:08Z

core/src/main/java/org/apache/calcite/avatica/remote/MockJsonService.java

+              + "[\"PUBLIC\",\"SCOTT\",\"EMP\",\"TABLE\",null,null,null,null,null,null,\"MEMORY\",false,null,\"EXTRA_LABEL2\"],"
+              + "[\"PUBLIC\",\"SCOTT\",\"BONUS\",\"TABLE\",null,null,null,null,null,null,\"MEMORY\",false,null,\"EXTRA_LABEL3\"],"
+              + "[\"PUBLIC\",\"SCOTT\",\"SALGRADE\",\"TABLE\",null,null,null,null,null,null,\"MEMORY\",false,null,null]]},"
+            + "\"updateCount\":-1,\"rpcMetadata\":null}"


This JSON blob is impenetrable. How did you produce this?

I ran another test within Avatica and took that json blob and modified it
What does 'impenetrable' mean here?

It's hard to read this data fixture and understand its direct relationship with assertions in tests.

There's also the usual problem with mocking - how do you keep your mocks in sync with the real implementation. When Avatica changes its response, tests depending on this fixture will go stale without further notice and there is no forcing function to cause them to be updated until or unless a regression occurs. But this problem is hard to solve without designing for it.

So one thing to note is that this test really just shows that whatever Signature comes in (different columns, different orders), Avatica is able to handle the different shapes and read them in the supplied order.
It doesn't test whether the real implementation is providing a correct response in the right order. That part should be up to the implementation.

Maybe that point wasn't clear by this additional test, perhaps I should shorten the JSON to contain fewer columns and add a comment stating that the point is that the signature column order matches up with the resultset columns?

wnob · 2023-09-12T22:40:56Z

server/src/test/java/org/apache/calcite/avatica/RemoteDriverMockTest.java

@@ -127,6 +127,21 @@ private Connection getMockConnection() {
    connection.close();
  }

+  @Test public void testTablesAdditionalColumns() throws Exception {


I'm not convinced of the usefulness of this test. Which part of the production code is actually being exercised? Kinda looks like we're just stubbing a mock, then testing that we stubbed it.

It mocks getting the response, but it actually tests the client code in RemoteMeta that handles the response and parses the signature into the ResultSet.

barrkel · 2023-09-14T13:33:06Z

core/src/main/java/org/apache/calcite/avatica/MetaImpl.java

@@ -388,6 +388,32 @@ public MetaColumn(
    public String getName() {
      return columnName;
    }
+    public static String[] getColumnNames() {


It's static so it can't be overridden, the reader thinks. The fact that the caller is using reflection to find and invoke it could definitely do with some commentary, if that is indeed the best way for the caller to get this information.

core/src/main/java/org/apache/calcite/avatica/MetaImpl.java

barrkel · 2023-09-14T13:43:40Z

core/src/main/java/org/apache/calcite/avatica/remote/MockJsonService.java

+              + "[\"PUBLIC\",\"SCOTT\",\"EMP\",\"TABLE\",null,null,null,null,null,null,\"MEMORY\",false,null,\"EXTRA_LABEL2\"],"
+              + "[\"PUBLIC\",\"SCOTT\",\"BONUS\",\"TABLE\",null,null,null,null,null,null,\"MEMORY\",false,null,\"EXTRA_LABEL3\"],"
+              + "[\"PUBLIC\",\"SCOTT\",\"SALGRADE\",\"TABLE\",null,null,null,null,null,null,\"MEMORY\",false,null,null]]},"
+            + "\"updateCount\":-1,\"rpcMetadata\":null}"


It's hard to read this data fixture and understand its direct relationship with assertions in tests.

There's also the usual problem with mocking - how do you keep your mocks in sync with the real implementation. When Avatica changes its response, tests depending on this fixture will go stale without further notice and there is no forcing function to cause them to be updated until or unless a regression occurs. But this problem is hard to solve without designing for it.

[CALCITE-5964] Add test case checking that connection.getMetaData().g…

b75a97f

…etTables() handles responses with different signature shapes. Also adding in Type.CLASS to ConnectionProperty.java and the associated getters.

olivrlee force-pushed the CALCITE-5964/metadata branch from eb50d7e to 143e336 Compare September 11, 2023 23:43

olivrlee marked this pull request as ready for review September 11, 2023 23:45

Update MockProtobufService to pass my RemoteDriverMockTest

d3e6a9f

Add in rows fix lint

olivrlee force-pushed the CALCITE-5964/metadata branch from 143e336 to d3e6a9f Compare September 11, 2023 23:56

wnob reviewed Sep 12, 2023

View reviewed changes

barrkel reviewed Sep 14, 2023

View reviewed changes

Remove Type.CLASS changes

af4e00b

F21 force-pushed the main branch 2 times, most recently from b97b819 to 4c0999b Compare November 28, 2023 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-5964] Add test case checking that connection.getMetaData().getTables() handles responses with different signature shapes. #227

[CALCITE-5964] Add test case checking that connection.getMetaData().getTables() handles responses with different signature shapes. #227

olivrlee commented Sep 11, 2023

wnob Sep 12, 2023

julianhyde Oct 24, 2023

wnob Sep 12, 2023

wnob Sep 12, 2023

barrkel Sep 14, 2023

wnob Sep 12, 2023

olivrlee Sep 13, 2023

barrkel Sep 14, 2023

olivrlee Sep 15, 2023

wnob Sep 12, 2023

olivrlee Sep 13, 2023

barrkel Sep 14, 2023

barrkel Sep 14, 2023

[CALCITE-5964] Add test case checking that connection.getMetaData().getTables() handles responses with different signature shapes. #227

Are you sure you want to change the base?

[CALCITE-5964] Add test case checking that connection.getMetaData().getTables() handles responses with different signature shapes. #227

Conversation

olivrlee commented Sep 11, 2023

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