python-pinot-dbapi / pinot-dbapi

Python DB-API and SQLAlchemy dialect for Pinot
MIT License
20 stars 33 forks source link

How does pinotdb handle Reserved words? #16

Closed 0902horn closed 3 years ago

0902horn commented 3 years ago

Hi, I'm trying to integrate Pinot with Superset and being blocked by querying columns that same as reserved words. Pinot SQL has some reserved words, like ‘time’ ‘timestamp’ etc. . A query using unquoted columns as such will cause a parser error. So how do we let Superset quote such columns using pinotdb?

I tried setting variables of PinotEngineSpec in Superset, but took no effect. On the other hand, I found that pydruid and [kylinpy]https://github.com/Kyligence/kylinpy/blob/master/kylinpy/sqla_dialect.py#L24 handle reserved words.

So could pinotd handle reserved words like pydruid and kylinpy?

0902horn commented 3 years ago

Pinotdb uses default. IdentifierPreparer which does not recognize 'time' , 'timestamp' etc. as reserved words. See https://github.com/sqlalchemy/sqlalchemy/blob/47c4fed1bc13600e98231a1474e36ce9a4750dd6/lib/sqlalchemy/sql/compiler.py#L47

xiangfu0 commented 3 years ago

you can use double quotes for identifiers.

0902horn commented 3 years ago

@xiangfu0 Thanks for reply. I know that reserved words should be quoted, but Superset chart offers no way to quote reserved words by myself if column names conflict with reserved words. I expect that pinotdb handles the case automatically since compiler.IdentifierPreparer is ready for this need.

0902horn commented 3 years ago

BTW, I'm trying to use compiler.IdentifierPreparer to quote identifiers. Still working on it.

xiangfu0 commented 3 years ago

@xiangfu0 Thanks for reply. I know that reserved words should be quoted, but Superset chart offers no way to quote reserved words by myself if column names conflict with reserved words. I expect that pinotdb handles the case automatically since compiler.IdentifierPreparer is ready for this need.

for superset, you can edit a column and in the expression, you can change it to "time"

0902horn commented 3 years ago

@xiangfu0 Thanks for reply. I know that reserved words should be quoted, but Superset chart offers no way to quote reserved words by myself if column names conflict with reserved words. I expect that pinotdb handles the case automatically since compiler.IdentifierPreparer is ready for this need.

for superset, you can edit a column and in the expression, you can change it to "time"

I think Superset uses sqlalchemy to generate SQL statement which is provided by pinotdb internally, and executes the statement via pinotdb. No matter what I did to Superset, I got the final SQL statement looks like this

SELECT text, time FROM event_test.event_test LIMIT 5

Then I got the following exception from Pinot

{'errorCode': 150,
 'message': 'PQLParsingError:\n'
            'org.apache.pinot.sql.parsers.SqlCompilationException: Caught '
            'exception while parsing query: SELECT text, time FROM '
            'event_test.event_test LIMIT 5\n'
            '\tat '
            'org.apache.pinot.sql.parsers.CalciteSqlParser.compileCalciteSqlToPinotQuery(CalciteSqlParser.java:324)\n'
            '\tat '
            'org.apache.pinot.sql.parsers.CalciteSqlParser.compileToPinotQuery(CalciteSqlParser.java:108)\n'
            '\tat '
            'org.apache.pinot.sql.parsers.CalciteSqlCompiler.compileToBrokerRequest(CalciteSqlCompiler.java:35)\n'
            '\tat '
            'org.apache.pinot.core.requesthandler.PinotQueryParserFactory.parseSQLQuery(PinotQueryParserFactory.java:46)\n'
            '\tat '
            'org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleSQLRequest(BaseBrokerRequestHandler.java:212)\n'
            '\tat '
            'org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleRequest(BaseBrokerRequestHandler.java:194)\n'
            '\tat '
            'org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleRequest(BaseBrokerRequestHandler.java:99)\n'
            '\tat '
            'org.apache.pinot.broker.api.resources.PinotClientRequest.processSqlQueryPost(PinotClientRequest.java:175)\n'
            '\tat '
            'jdk.internal.reflect.GeneratedMethodAccessor29.invoke(Unknown '
            'Source)\n'
            '\tat '
            'java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)\n'
            '\tat java.base/java.lang.reflect.Method.invoke(Method.java:566)\n'
            '\tat '
            'org.glassfish.jersey.server.model.internal.ResourceMethodInvocationHandlerFactory.lambda$static$0(ResourceMethodInvocationHandlerFactory.java:52)\n'
            '\tat '
            'org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher$1.run(AbstractJavaResourceMethodDispatcher.java:124)\n'
            '\tat '
            'org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.invoke(AbstractJavaResourceMethodDispatcher.java:167)'}

So I opened a PR #17 to fix this in pinotdb. The final SQL statement will look like this

SELECT text, "time" FROM event_test.event_test LIMIT 5
xiangfu0 commented 3 years ago

Thanks! I merged your change and published release 0.3.6.

0902horn commented 3 years ago

I tested pinotdb 0.3.6 with superset. It works as expected.