clintropolis commented on code in PR #12709: URL: https://github.com/apache/druid/pull/12709#discussion_r917139745
########## sql/src/main/java/org/apache/druid/sql/SqlRequest.java: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import org.apache.calcite.avatica.remote.TypedValue; +import org.apache.druid.query.QueryContext; +import org.apache.druid.server.security.AuthenticationResult; +import org.apache.druid.sql.http.SqlParameter; +import org.apache.druid.sql.http.SqlQuery; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +/** + * Captures the inputs to a SQL execution request: the statement, + * the context, parameters, and the authorization result. Pass this + * around rather than the quad of items. The request can evolve: + * items can be filled in later as needed (except for the SQL + * and auth result, which are required.) + */ +public class SqlRequest Review Comment: unsure on naming, seems a bit strange to me for something with request in the name to have things called result inside of it. I think since `SqlQuery` is the HTTP request object and there is a method to create one of these from one of those it adds to the perceived strangeness of the naming. In some ways this seems like the JDBC analog of `SqlQuery`, though also with the auth information decorated on it, should we swap the names of `SqlQuery` and `SqlRequest`? or call this `DruidQueryStatement` .. something? idk naming is hard ########## sql/src/main/java/org/apache/druid/sql/SqlRequest.java: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import org.apache.calcite.avatica.remote.TypedValue; +import org.apache.druid.query.QueryContext; +import org.apache.druid.server.security.AuthenticationResult; +import org.apache.druid.sql.http.SqlParameter; +import org.apache.druid.sql.http.SqlQuery; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +/** + * Captures the inputs to a SQL execution request: the statement, + * the context, parameters, and the authorization result. Pass this + * around rather than the quad of items. The request can evolve: + * items can be filled in later as needed (except for the SQL + * and auth result, which are required.) + */ +public class SqlRequest +{ + private final String sql; + private final QueryContext queryContext; + private final List<TypedValue> parameters; + private final AuthenticationResult authResult; + + public SqlRequest( + String sql, + QueryContext queryContext, + List<TypedValue> parameters, + AuthenticationResult authResult + ) + { + this.sql = Preconditions.checkNotNull(sql); + this.queryContext = queryContext == null + ? new QueryContext(ImmutableMap.of()) Review Comment: nit: why not just use `new QueryContext()`? it makes empty user parameters so this seems unecessary ########## sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java: ########## @@ -108,22 +109,27 @@ import java.sql.Timestamp; import java.sql.Types; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Properties; import java.util.Set; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import java.util.concurrent.ThreadLocalRandom; -public abstract class DruidAvaticaHandlerTest extends CalciteTestBase +/** + * Tests Avatica using JSON serialization. Review Comment: Should this just be framed meant to be a generic set of avatica jdbc tests with a default implementation that tests JSON, since `DruidAvaticaProtobufHandlerTest` uses this too? ########## sql/src/main/java/org/apache/druid/sql/SqlRequest.java: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import org.apache.calcite.avatica.remote.TypedValue; +import org.apache.druid.query.QueryContext; +import org.apache.druid.server.security.AuthenticationResult; +import org.apache.druid.sql.http.SqlParameter; +import org.apache.druid.sql.http.SqlQuery; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +/** + * Captures the inputs to a SQL execution request: the statement, + * the context, parameters, and the authorization result. Pass this + * around rather than the quad of items. The request can evolve: + * items can be filled in later as needed (except for the SQL + * and auth result, which are required.) + */ +public class SqlRequest +{ + private final String sql; + private final QueryContext queryContext; + private final List<TypedValue> parameters; + private final AuthenticationResult authResult; + + public SqlRequest( + String sql, + QueryContext queryContext, + List<TypedValue> parameters, + AuthenticationResult authResult + ) + { + this.sql = Preconditions.checkNotNull(sql); + this.queryContext = queryContext == null + ? new QueryContext(ImmutableMap.of()) + : queryContext; + this.parameters = parameters == null + ? Collections.emptyList() + : parameters; + this.authResult = Preconditions.checkNotNull(authResult); + } + + public SqlRequest(final String sql, final AuthenticationResult authResult) + { + this(sql, (QueryContext) null, null, authResult); + } + + public static SqlRequest fromQuery(SqlQuery sqlQuery, final AuthenticationResult authResult) Review Comment: is this used anywhere? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
