Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-4121] Driver misplaces properties from URL while connecting (Laksh Singla) #162

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LakshSingla
Copy link

Driver.java fails to pass the parameters passed via connection URL to the OpenConnectionRequest. This PR introduces changes which extract the context properties, and pass it to the OpenConnectionRequest call.

Fixes: apache/druid#10184
Resolves: https://issues.apache.org/jira/browse/CALCITE-4121

@joshelser
Copy link
Member

Thanks for trying to fix this, @LakshSingla

I'm not 100% certain anymore, but this may have been an intentional decision previously. Avatica has its own set of JDBC properties which can be passed into the driver which control how the Avatica JDBC driver can talk to the Avatica server. These properties can be specified either in Java via the Properties object in DriverManager or via the JDBC url itself as key-value pairs.

https://calcite.apache.org/avatica/docs/client_reference.html

In addition to that, there are also JDBC properties which the Avatica server (with the real backend JDBC driver) may want/need to talk to the backend database. I believe your hope is to make it such that Avatica clients could directly pass JDBC properties to the backend database.

I remember a worry I had: "what if a property name we use for Avatica conflicts with a property for the backend database?". I could see past-Josh having made the decision to not directly pass properties from client, through Avatica server, to the backend database.

Would it be possible for Avatica to only pass Properties which it does not know about into the backend database rather than all Properties?

We definitely need some test-case coverage here. We use HSQLDB for all of the unit tests against a "real" database, but I do not see any option which you could provide to do an end-to-end test case http://hsqldb.org/doc/2.0/guide/dbproperties-chapt.html#N16054

I would suggest you start looking in https://github.com/apache/calcite-avatica/blob/master/server/src/test/java/org/apache/calcite/avatica/RemoteDriverTest.java to see if there is a natural place you can validate the server-side Connection Properties. https://github.com/apache/calcite-avatica/blob/master/server/src/test/java/org/apache/calcite/avatica/remote/ConnectionPropertiesTest.java may also be helpful. There are not too many test classes in the server module -- that is where you should look.

@LakshSingla
Copy link
Author

Thanks for the insightful comment @joshelser. Before I try to make any code changes, I wanted to check if my understanding is correct.

There are 2 types of properties:

  1. Those which define how the Avatica driver communicates with the database.
  2. Custom properties from the Avatica client to the server.

According to your comment, it seems that the first set of properties could be passed in a) JDBC URL or b). While getting the connection from DriverManager as DriverManager.getConnection(url, PROPERTIES), but there is no provision for setting the second set of custom properties from client to the DB. As per my understanding however, the second set of custom properties can be passed in the properties object, but not via the URL.
Is this behaviour intentional or accidental?

@F21 F21 force-pushed the main branch 2 times, most recently from b97b819 to 4c0999b Compare November 28, 2023 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants