git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Avatica or Calcite or mine fault?


An AvaticaParameter with a null classname sounds wrong to me. We should know the type (per the schema) for some variable, regardless of whether or not it is nullable.

If you can put together a unit test showing what you're seeing in Avatica, that would go an extremely long way. It may also just be something Calcite is doing which should be corrected (so, a unit test there would be a starting point, too).

Feel free to ping me on Jira. Thanks Piotr!

On 10/4/18 9:08 AM, ptr.bojko@xxxxxxxxx wrote:
I've logged a JIRA for the first bug, with a fix proposal -
https://issues.apache.org/jira/browse/CALCITE-2609.

But the case with AvaticaParameter NPE still remains unclear for me - in
terms of design. Help needed :)

On Wed, Oct 3, 2018 at 4:14 PM ptr.bojko@xxxxxxxxx <ptr.bojko@xxxxxxxxx>
wrote:

As for the nulls in AvaticaParameter class - I believe this is how calcite
works. For example for the following query *"select * from filters where
name = ?"* - despite the definition of filters table - Calcite will
create org.apache.calcite.avatica.AvaticaParameter with nullable className.

My case with more details is following: based on calcite-avatica/sever
I've created a mini jdbc server (servlet) exposing Calcite connection with
my custom schemas. I would like to expose my domain as an SQL database so
with Calcite as an engine and domain translation and Avatica as a JDBC
implementor the resulting implementation is very elegant and works great.

However the same  query *"select * from filters where name = ?" *run
through avatica jdbc implementation is resulting NPE
in Common.AvaticaParameter AvaticaParameter.toProto().

On Wed, Oct 3, 2018 at 12:46 AM Josh Elser <elserj@xxxxxxxxxx> wrote:

re: nulls on className, that's how Protobuf itself works. You can't set
null for an attribute.

What is the circumstance where you would have a null className on an
AvaticaParameter?

We could proactively check in the constructor to AvaticaParameter that
you don't provide null arguments, but that could be argued in either
direction (e.g. you should not provide null arguments in the first place).

On 10/2/18 5:11 PM, ptr.bojko@xxxxxxxxx wrote:
Hello,

It seems that my case consists of two bugs:

First one is similar to what I've already patched twice - Calcite pushes
too much to underlying jdbc schema, in this case "?" operator. Resulting
subquery onto jdbc schema ends with error about it (? not resolved).
This
one belongs to Calcite.

The second one is located
at org.apache.calcite.avatica.AvaticaParameter.toProto method.
AvaticaParameter can be a live nullable className but
Common.AvaticaParameter does not allow to set nulls on className.
toProto
method - please verify me - should be more carefull about it and not set
className on a builder when it is null. This one belongs to Avatica.

If anyone has time to check me it would be great. I will log then each
bug
to appropriate apache project in Jira. I should patch first one fairly
easy.

Regards,
Piotr

On Mon, Oct 1, 2018 at 1:50 PM Francis Chuang <francischuang@xxxxxxxxxx

wrote:

Hey Piotr,

Thanks for reporting this! I am not familiar with Avatica's internals,
so can't recommend how this can be fixed. However, I would suggest
writing a test case to reproduce the problem in the meantime.

Francis

On 1/10/2018 8:59 PM, ptr.bojko@xxxxxxxxx wrote:
Hello fellow calcite dev team :)

I have discovered the case with NPE when trying to use parameters on
prepared statement:
java.lang.NullPointerException
at


org.apache.calcite.avatica.proto.Common$AvaticaParameter$Builder.setClassName(Common.java:9040)
at


org.apache.calcite.avatica.AvaticaParameter.toProto(AvaticaParameter.java:64)
at org.apache.calcite.avatica.Meta$Signature.toProto(Meta.java:835)
at
org.apache.calcite.avatica.Meta$StatementHandle.toProto(Meta.java:1236)
at


org.apache.calcite.avatica.remote.Service$PrepareResponse.serialize(Service.java:1310)
at


org.apache.calcite.avatica.remote.Service$PrepareResponse.serialize(Service.java:1275)
at


org.apache.calcite.avatica.remote.ProtobufTranslationImpl.serializeResponse(ProtobufTranslationImpl.java:348)
at


org.apache.calcite.avatica.remote.ProtobufHandler.encode(ProtobufHandler.java:57)
at


org.apache.calcite.avatica.remote.ProtobufHandler.encode(ProtobufHandler.java:31)
at


org.apache.calcite.avatica.remote.AbstractHandler.apply(AbstractHandler.java:95)
at


org.apache.calcite.avatica.remote.ProtobufHandler.apply(ProtobufHandler.java:46)


It looks like CalcitePrepareImpl class does have one method
implemented:
      private static String getClassName(RelDataType type) {
           return null;
       }
Or Common$AvaticaParameter$Builder.setClassName is too restrictive? Or
maybe AvaticaParameter.toProto() should not feed the builder with
nullable
className?

Please advice, so I could help patch this.







--
Piotr Bojko
http://about.me/ptr.bojko