Add context appender to sql based connectors #14500

s2lomon · 2022-10-06T11:56:59Z

Description

Add context appender to sql based connectors

This is a first iteration for SELECT Queries of a mechanism
that will allow to send to the actual database a bit more
information, that might be available only in Trino. It's for the
purpose of these other systems to be able to capture this info and
log it for its own purposes. For now we are sending it as a comments
added at the end of a query.

Non-technical explanation

This pr adds an ability to add additional context of query execution and send it to external database,
like pgsql, oracle etc.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Add context comments to SELECT queries in sql based connectors.

...rino-postgresql/src/test/java/io/trino/plugin/postgresql/TestQueryContextLoggingEnabled.java

skrzypo987

Added some comments

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java

skrzypo987 · 2022-10-07T05:41:30Z

...e-jdbc/src/main/java/io/trino/plugin/jdbc/QueryExecutionConnectorSessionContextAppender.java

+
+import io.trino.spi.connector.ConnectorSession;
+
+public interface QueryExecutionConnectorSessionContextAppender


Name is confusing. I don't really understand the "connector" part.

It's from ConnectorSession But maybe it would be better to change it to QueryExecutionContextAppender

...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/QueryCommentAsContextAppender.java

findepi · 2022-10-07T10:19:23Z

Its work in progress - shared just to gain feedback and start discussion on the approach.

Awesome!

What does this PR aim to solve?

findepi · 2022-10-07T10:20:09Z

Description

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required. ( ) Release notes are required, please propose a release note for me. ( ) Release notes are required, with the following suggested text:
# Section
* Fix some things. ({issue}`issuenumber`)

if you don't feel like filling this in, just remove it.
it will make my life easier as i don't need to read text that carries no information

thank you!

findepi

just skimming.

not possible to do better review without knowing what is the purpose of the code

findepi · 2022-10-07T10:21:39Z

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestingPostgreSqlServer.java

    private static final String LOG_CANCELLATION_EVENT = "ERROR:  canceling statement due to user request";
+
+    private static final Pattern SQL_QUERY_FIND_PATTERN = Pattern.compile("^(: |/C_\\d: )(.*)");


May be worth adorning this with example log line we're parsing

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestingPostgreSqlServer.java

findepi · 2022-10-07T10:23:29Z

...rino-postgresql/src/test/java/io/trino/plugin/postgresql/TestQueryContextLoggingEnabled.java

+public class TestQueryContextLoggingEnabled
+        extends AbstractTestQueryFramework
+{
+    private final TestingPostgreSqlServer postgreSqlServer = closeAfterClass(new TestingPostgreSqlServer());


Never allocate resources or do IO in a test static or instance initializer.

in case of failure, surefire we'll be glad to misguide you on what happened

you use precious memory which is needed only for the test's duration

hashhar · 2022-10-10T08:37:06Z

Some context would be useful when reviewing this (to understand expected usage).

s2lomon

Thanks for your responses, please take a look again.

s2lomon · 2022-10-13T15:41:12Z

...e-jdbc/src/main/java/io/trino/plugin/jdbc/QueryExecutionConnectorSessionContextAppender.java

+
+import io.trino.spi.connector.ConnectorSession;
+
+public interface QueryExecutionConnectorSessionContextAppender


It's from ConnectorSession But maybe it would be better to change it to QueryExecutionContextAppender

...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/QueryCommentAsContextAppender.java

...rino-postgresql/src/test/java/io/trino/plugin/postgresql/TestQueryContextLoggingEnabled.java

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestingPostgreSqlServer.java

hashhar · 2022-10-13T17:34:10Z

I really hope people won't abuse this to implement passing query hints to Oracle.

Will take a look at the PR.

skrzypo987 · 2022-10-14T11:45:08Z

...rino-postgresql/src/test/java/io/trino/plugin/postgresql/TestQueryContextLoggingEnabled.java

+        DistributedQueryRunner distributedQueryRunner = createPostgreSqlQueryRunner(
+                postgreSqlServer,
+                Map.of(),
+                Map.of("comment-logging.format", "query executed by $USER"),


Add all variables here.

It will be hard to use them, as some of them are generated automatically (query_id) and some of them are not provided (as they would have to come from the client itself, like trace_token and source). It's tested extensively elsewhere.

core/trino-main/src/main/java/io/trino/testing/TestingConnectorSession.java

kokosing · 2022-10-14T12:19:40Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryExecutionContextAppender.java

+    QueryExecutionContextAppender NO_CONTEXT_APPENDER = (query, session) -> query;
+
+    String appendContext(String query, ConnectorSession session)
+            throws IllegalArgumentException;


remove throws

Why? That's how you notify the clients that this method can actually throws something and it's by design. What's more you are showing to the implementors what kind of exceptions can be thrown, without the need of adding additional handling.

IllegalArgumentException can be expected from any method.

Ok.

What do you mean by "Ok"? I was already preparing popcorn and awaiting epic philosophical battle. Try be more assertive next time, other reviewers demand entertainment.

Ha. Ha. Ha. ;)

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryExecutionContextAppender.java

.../trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/QueryCommentContextAppender.java

...-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/QueryCommentContextAppenderConfig.java

...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/QueryCommentAsContextAppender.java

core/trino-main/src/main/java/io/trino/testing/TestingConnectorSession.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java

.../trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/QueryCommentContextAppender.java

...-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/QueryCommentContextAppenderModule.java

...no-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestQueryCommentContextAppender.java

Praveen2112 · 2022-10-14T16:09:35Z

What if we implement a ForwardingQueryBuilder which appends this context ? But we need to wrap all QueryBuilder impl with this new impl.

s2lomon

Please take a look now. I've made a 3 commits but I will squash them into a single one later on.

s2lomon · 2022-10-20T08:40:52Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryExecutionContextAppender.java

+    QueryExecutionContextAppender NO_CONTEXT_APPENDER = (query, session) -> query;
+
+    String appendContext(String query, ConnectorSession session)
+            throws IllegalArgumentException;


.../trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/QueryCommentContextAppender.java

s2lomon · 2022-10-21T12:41:51Z

...rino-postgresql/src/test/java/io/trino/plugin/postgresql/TestQueryContextLoggingEnabled.java

+        DistributedQueryRunner distributedQueryRunner = createPostgreSqlQueryRunner(
+                postgreSqlServer,
+                Map.of(),
+                Map.of("comment-logging.format", "query executed by $USER"),


It will be hard to use them, as some of them are generated automatically (query_id) and some of them are not provided (as they would have to come from the client itself, like trace_token and source). It's tested extensively elsewhere.

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestingPostgreSqlServer.java

kokosing

looks good. I reviewed everything at once (not commit by commit).

kokosing · 2022-10-24T08:46:23Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

@@ -658,7 +665,7 @@ public JdbcOutputTableHandle beginInsertTable(ConnectorSession session, JdbcTabl
        }
    }

-    protected void copyTableSchema(Connection connection, String catalogName, String schemaName, String tableName, String newTableName, List<String> columnNames)
+    protected void copyTableSchema(Connection connection, String catalogName, String schemaName, String tableName, String newTableName, List<String> columnNames, ConnectorSession session)


nit: ConnectorSession is usually passed as first argument. Please apply that to all places.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

kokosing · 2022-10-24T08:49:46Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+            log.debug("Execute: %s", appendedQuery);
+            statement.execute(appendedQuery);
+        }
+        catch (IllegalArgumentException e) {


Do we need this catch? Is this an user, admin or developer error? The same comment to other places like that.

It's potential SQLInject attack, that's why I want to handle it explicitly with JDBC_NON_TRANSIENT_ERROR - meaning that this SQL should not be repeated as the error won't go away afer a while and there is no point of repeating the query. You need to change the client setup to make it disappear.

Then I suggest that the query modifier should throw proper exception directly or return Optional should be used.

So Optional.empty() would mean that you can execute code without context being logged, even though configuration says that it will be logged. That's a security risk as well. I think that we should be fine throwing this exception directly though.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryExecutionContextAppender.java

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestingPostgreSqlServer.java

...trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestRemoteQueryCommentLogging.java

skrzypo987

lgtm % kokosing remarks

skrzypo987 · 2022-10-24T09:52:58Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryExecutionContextAppender.java

+    QueryExecutionContextAppender NO_CONTEXT_APPENDER = (query, session) -> query;
+
+    String appendContext(String query, ConnectorSession session)
+            throws IllegalArgumentException;


Ok.

What do you mean by "Ok"? I was already preparing popcorn and awaiting epic philosophical battle. Try be more assertive next time, other reviewers demand entertainment.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClient.java

skrzypo987 · 2022-10-24T10:38:07Z

Change the PR description to cover the current scope

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifierConfig.java

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifierModule.java

plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcClient.java

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestOracleClient.java

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

...trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestRemoteQueryCommentLogging.java

s2lomon

Thanks for review, changes are not yet done, but please take a look at the comments.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

s2lomon · 2022-10-26T10:55:17Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+            log.debug("Execute: %s", appendedQuery);
+            statement.execute(appendedQuery);
+        }
+        catch (IllegalArgumentException e) {


It's potential SQLInject attack, that's why I want to handle it explicitly with JDBC_NON_TRANSIENT_ERROR - meaning that this SQL should not be repeated as the error won't go away afer a while and there is no point of repeating the query. You need to change the client setup to make it disappear.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryExecutionContextAppender.java

s2lomon · 2022-10-26T11:15:38Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifier.java

+        for (PredefinedValue predefinedValue : PredefinedValue.values()) {
+            message = message.replaceAll(predefinedValue.getMatchCase(), predefinedValue.value(session));
+        }
+        return query + "/*" + message + "*/";


Hm ok it will look better in logs and I guess humans will be reading it with the query.

...trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestRemoteQueryCommentLogging.java

s2lomon · 2022-10-26T11:36:42Z

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestingPostgreSqlServer.java

@@ -88,6 +93,18 @@ private static void execute(String url, Properties properties, String sql)
        }
    }

+    DatabaseEvents startCollecting()


plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestingPostgreSqlServer.java

kokosing · 2022-10-26T14:37:47Z

I am fine with responses, let me know when you need another round of the review.

s2lomon

All comments addressed. Please check again.

s2lomon · 2022-11-02T12:50:11Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+            log.debug("Execute: %s", appendedQuery);
+            statement.execute(appendedQuery);
+        }
+        catch (IllegalArgumentException e) {


So Optional.empty() would mean that you can execute code without context being logged, even though configuration says that it will be logged. That's a security risk as well. I think that we should be fine throwing this exception directly though.

...trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestRemoteQueryCommentLogging.java

Praveen2112 · 2022-11-08T11:26:16Z

It still shows some conflict, can you resolve them.

kokosing

Minor comments.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

kokosing · 2022-11-12T20:43:51Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSink.java

            statement = connection.prepareStatement(insertSql);
        }
+        catch (TrinoException e) {
+            throw closeAllSuppress(e, connection);


I would throw new TrinoException with e as cause. So all you need to do is catch (SQLException | TrinoException e)

I don't think that it's correct from the exception handling stnadnpoint. We already have the exception and there is no need to wrap it again without adding any meaningful information to the caller. Plus we are not returning JDBC_ERROR but rather NON_TRANSIENT_JDBC_ERROR wich is the whole point of this exception being thrown.

kokosing · 2022-11-12T20:47:25Z

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifierConfig.java

+
+import static java.util.stream.Collectors.joining;
+
+public class RemoteQueryModifierConfig


Maybe inline this class into QueryConfig?

I don't like this idea. It has quite a lot of tests and there is no point in adding these tests to default jdbc config tests as they are very specific

kokosing · 2022-11-12T20:47:38Z

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifierConfig.java

+    private static final Pattern VALIDATION_PATTERN = Pattern.compile("[\\w ,=]|" + String.join("|", PREDEFINED_MATCHES));
+    private String format = "";
+
+    @Config("remote-query.comment.format")


query.comment-format?

I think that remote-query is better, as it tells exactly which query we are talking about in this case. comment-format might be a bit closer to what we do in other places. so remote-query.comment-format?

Then I think we need to change query.reuse-connection. I am fine with this. However then the scope of this PR gets bigger.

Also I know we are all aware of concept of "remote query". I am not sure if it is well known to our users. There is a risk where "remote" part may confuse them.

Ok I will follow your lead here.

kokosing · 2022-11-12T20:49:52Z

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifierConfig.java

+
+    @Config("remote-query.comment.format")
+    @ConfigDescription("Format in which logs about query execution context should be added as comments sent through jdbc driver." +
+            " It can consist only of letters, digits, spaces, underscores, equality signs and predefined values: $QUERY_ID, $SOURCE, $USER, $TRACE_TOKEN")


It is too long. Let's add details to docs and here let's be short.

Message format which will be added as comment to remote query.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifier.java

kokosing · 2022-11-12T20:54:08Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestRemoteQueryModifier.java

+
+    private static RemoteQueryModifier createRemoteQueryModifier(String commentFormat)
+    {
+        return new RemoteQueryModifier(new RemoteQueryModifierConfig().setFormat(commentFormat));


Maybe add a constructor that takes only format string?

I could and I could make it package-private to not be used outside of this package.

s2lomon

please take a look at my responses. I will append change shortly.

s2lomon · 2022-11-15T13:02:21Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSink.java

            statement = connection.prepareStatement(insertSql);
        }
+        catch (TrinoException e) {
+            throw closeAllSuppress(e, connection);


I don't think that it's correct from the exception handling stnadnpoint. We already have the exception and there is no need to wrap it again without adding any meaningful information to the caller. Plus we are not returning JDBC_ERROR but rather NON_TRANSIENT_JDBC_ERROR wich is the whole point of this exception being thrown.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifier.java

s2lomon · 2022-11-15T14:10:15Z

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifierConfig.java

+    private static final Pattern VALIDATION_PATTERN = Pattern.compile("[\\w ,=]|" + String.join("|", PREDEFINED_MATCHES));
+    private String format = "";
+
+    @Config("remote-query.comment.format")


I think that remote-query is better, as it tells exactly which query we are talking about in this case. comment-format might be a bit closer to what we do in other places. so remote-query.comment-format?

s2lomon · 2022-11-15T14:10:28Z

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifierConfig.java

+
+    @Config("remote-query.comment.format")
+    @ConfigDescription("Format in which logs about query execution context should be added as comments sent through jdbc driver." +
+            " It can consist only of letters, digits, spaces, underscores, equality signs and predefined values: $QUERY_ID, $SOURCE, $USER, $TRACE_TOKEN")


s2lomon · 2022-11-15T14:11:16Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestRemoteQueryModifier.java

+
+    private static RemoteQueryModifier createRemoteQueryModifier(String commentFormat)
+    {
+        return new RemoteQueryModifier(new RemoteQueryModifierConfig().setFormat(commentFormat));


I could and I could make it package-private to not be used outside of this package.

Praveen2112

It would be nice if we could have test for default behaviour. Overall design looks good to me

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifier.java

Praveen2112 · 2022-11-16T10:50:11Z

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifierConfig.java

+        return this;
+    }
+
+    public String getFormat()


Should we have a NotNull annotation ?

Praveen2112 · 2022-11-16T10:54:10Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/RemoteQueryModifier.java

+        for (PredefinedValue predefinedValue : PredefinedValue.values()) {
+            message = message.replaceAll(predefinedValue.getMatchCase(), predefinedValue.value(session));
+        }
+        return query + " /*" + message + "*/";


By default - we shouldn’t add any conext to the query. Won’t this add a /* NOT USED*/ for all queries.

No it would not in previous implementation and after the change it won't be an issue as well.

Praveen2112 · 2022-11-16T10:56:06Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestRemoteQueryModifier.java

+    }
+
+    @Test
+    public void testFormatWithEmptyValues()


Can we have an additional test case - for default behaviour (if format is not specified)

Praveen2112 · 2022-11-16T10:58:54Z

...rino-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestRemoteQueryModifierModule.java

+                .initialize()
+                .getInstance(RemoteQueryModifier.class);
+
+        assertThat(remoteQueryModifier)


Instead of asserting remoteQueryModifier - can we assert its behaviour.

...trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestRemoteQueryCommentLogging.java

Praveen2112 · 2022-11-16T11:01:37Z

...trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestRemoteQueryCommentLogging.java

+        return distributedQueryRunner;
+    }
+
+    @Test(singleThreaded = true)


Is there is any specific reason on why we didn’t annotate the TestClass with this ?

Praveen2112

Two minor change. LGTM

Praveen2112 · 2022-11-21T07:54:58Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java

+    }
+
+    @Inject
+    public DefaultQueryBuilder(RemoteQueryModifier contextAppender)


nit : Can rename the variable as queryModifier and in the rnn message

Sorry missed that last time.

Praveen2112 · 2022-11-21T08:08:32Z

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestingPostgreSqlServer.java

+            return server.getRemoteDatabaseEvents()
+                    .stream()
+                    .skip(startingFromEventsCount)
+                    .limit(lastEventsCount)


Should this be the difference between the starting and lastEventCount ?
BTW Do we need this bottom limit, since the test are single threaded and we already have a starting point.

It was a request from @kokosing to make it immutable. Thanks to this it is, as every method will always return same events. We are not using this fact at this moment in our tests, but it's still better to have it as such and don't be forced to think about it in the tests.

In this case, should limit be the difference between the startingEvent and lastEvents or should we re-order the streams ?

So I believe that this is correct, as skip doesn't truncate the stream, it just skips first few and limit limits all values, but to be honest, I would prefer to have a test for that to be 100% sure, and we don't have that here. Due to that, I've decided to rewrite this part quickly, to not confuse anyone and to have immutability down to the values identity.

s2lomon · 2022-11-21T12:25:36Z

I've added also a suite of tests checking for correct default behaviour - meaning that nothing should be logged at all when it's disabled. I've noticed that I have only partially tested that in guice setup tests.

Praveen2112 · 2022-11-22T09:01:38Z

CI is red. Can you please take a look.

This change allows for sending additional logs, that are send in comments to external systems (jdbc only atm). It allows to pass additional security and observability information to these systems, that wouldn't be possible to send otherwie. This implementation covers SELECT, DDL and DML queries, although it does not cover any metadata queries - as these are made by the drivers itself and we can't easily intercept these.

cla-bot bot added the cla-signed label Oct 6, 2022

kokosing reviewed Oct 6, 2022

View reviewed changes

...rino-postgresql/src/test/java/io/trino/plugin/postgresql/TestQueryContextLoggingEnabled.java Outdated Show resolved Hide resolved

...rino-postgresql/src/test/java/io/trino/plugin/postgresql/TestQueryContextLoggingEnabled.java Outdated Show resolved Hide resolved

skrzypo987 reviewed Oct 7, 2022

View reviewed changes

findepi reviewed Oct 7, 2022

View reviewed changes

s2lomon force-pushed the comment_query_appenders branch from c362b5d to d5aa7c1 Compare October 13, 2022 15:58

s2lomon commented Oct 13, 2022

View reviewed changes

kokosing changed the title ~~WIP comment_query_appenders~~ Add context appender to sql based connectors Oct 13, 2022

skrzypo987 reviewed Oct 14, 2022

View reviewed changes

kokosing reviewed Oct 14, 2022

View reviewed changes

Praveen2112 reviewed Oct 14, 2022

View reviewed changes

s2lomon commented Oct 21, 2022

View reviewed changes

s2lomon requested review from Praveen2112, skrzypo987, kokosing and findepi October 24, 2022 09:09

kokosing reviewed Oct 24, 2022

View reviewed changes

skrzypo987 reviewed Oct 24, 2022

View reviewed changes

Praveen2112 reviewed Oct 26, 2022

View reviewed changes

s2lomon commented Oct 26, 2022

View reviewed changes

s2lomon force-pushed the comment_query_appenders branch from 09bfc83 to 5db6244 Compare November 3, 2022 09:31

s2lomon commented Nov 3, 2022

View reviewed changes

s2lomon force-pushed the comment_query_appenders branch 2 times, most recently from a824a6b to 8f90f2e Compare November 3, 2022 12:23

findepi removed their request for review November 4, 2022 12:54

s2lomon requested review from Praveen2112 and findepi November 8, 2022 10:30

s2lomon force-pushed the comment_query_appenders branch from 7fe0d2b to f9b73fb Compare November 9, 2022 12:04

kokosing reviewed Nov 12, 2022

View reviewed changes

s2lomon commented Nov 15, 2022

View reviewed changes

Praveen2112 reviewed Nov 16, 2022

View reviewed changes

s2lomon force-pushed the comment_query_appenders branch 2 times, most recently from ef49450 to a86828e Compare November 17, 2022 16:47

Praveen2112 approved these changes Nov 21, 2022

View reviewed changes

s2lomon force-pushed the comment_query_appenders branch from a86828e to 52dc6f9 Compare November 21, 2022 12:17

s2lomon force-pushed the comment_query_appenders branch from 52dc6f9 to 91dc517 Compare November 21, 2022 21:59

s2lomon force-pushed the comment_query_appenders branch 2 times, most recently from 8303ff2 to 9ed7df5 Compare November 23, 2022 10:02

s2lomon force-pushed the comment_query_appenders branch from 9ed7df5 to e2771a8 Compare November 23, 2022 12:50

Trigger CI

77c99e3

Praveen2112 merged commit 79401db into trinodb:master Nov 24, 2022

github-actions bot added this to the 404 milestone Nov 24, 2022

colebow mentioned this pull request Nov 28, 2022

Add Trino 405 release notes #15139

Merged

This was referenced Jan 12, 2023

Minor cleanups in DefaultQueryBuilder #15688

Merged

Allow hyphen as a valid value for sanitization checks #15691

Merged

Apply sanitization checks only for entity specified in comment-format #15692

Merged

kokosing mentioned this pull request Feb 20, 2023

Label BigQuery jobs with Trino query id #16187

Merged

Jessie212 mentioned this pull request Feb 28, 2023

Add support for appending queries in sql based connectors #16308

Merged


		import io.trino.spi.connector.ConnectorSession;

		public interface QueryExecutionConnectorSessionContextAppender

		private static final String LOG_CANCELLATION_EVENT = "ERROR: canceling statement due to user request";

		private static final Pattern SQL_QUERY_FIND_PATTERN = Pattern.compile("^(: \|/C_\\d: )(.*)");


		import static java.util.stream.Collectors.joining;

		public class RemoteQueryModifierConfig

Add context appender to sql based connectors #14500

Add context appender to sql based connectors #14500

Conversation

s2lomon commented Oct 6, 2022 • edited Loading

Description

Non-technical explanation

Release notes

skrzypo987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Oct 7, 2022

findepi commented Oct 7, 2022

Description

Non-technical explanation

Release notes

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar commented Oct 10, 2022

s2lomon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar commented Oct 13, 2022

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

Praveen2112 commented Oct 14, 2022

s2lomon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a 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

skrzypo987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skrzypo987 commented Oct 24, 2022

s2lomon left a 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

kokosing commented Oct 26, 2022

s2lomon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Nov 8, 2022

kokosing left a 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

Choose a reason for hiding this comment

s2lomon left a 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

Praveen2112 left a comment

s2lomon commented Oct 6, 2022 •

edited

Loading