Add anonymized query plan in json format to QueryCompletedEvent

[gaurav8297]

Description

Add anonymized query plan in json format to QueryCompletedEvent
Implement EXPLAIN (TYPE DISTRIBUTED, FORMAT JSON)

Is this change a fix, improvement, new feature, refactoring, or other?

New feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Event listener SPI, EXPLAIN

How would you describe this change to a non-technical end user or system administrator?

Provides anonymised query plan in json format to event listener to enable offline analysis without leaking sensitive info.
Implements EXPLAIN (TYPE DISTRIBUTED, FORMAT JSON)

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# General
* Implement EXPLAIN (TYPE DISTRIBUTED, FORMAT JSON). ({issue}`12968`)
* Add anonymized query plan in json format to QueryCompletedEvent. ({issue}`12968`)

[raunaqmorarka]

Please add examples of anonymised plans to the PR

[raunaqmorarka]

Please add examples of anonymised plans to the PR

ping

[raunaqmorarka]

Test failures are related, PTAL

[gaurav8297]

Please add examples of anonymised plans to the PR

Query: SELECT quantity FROM lineitem LIMIT 10

{
  "0" : {
    "id" : "6",
    "name" : "Output",
    "descriptor" : {
      "columnNames" : "[column_717400177]"
    },
    "outputs" : [ {
      "symbol" : "symbol_717400177",
      "type" : "double"
    } ],
    "details" : [ ],
    "estimates" : [ ],
    "children" : [ {
      "id" : "98",
      "name" : "Limit",
      "descriptor" : {
        "count" : "10",
        "withTies" : "",
        "inputPreSortedBy" : ""
      },
      "outputs" : [ {
        "symbol" : "symbol_717400177",
        "type" : "double"
      } ],
      "details" : [ ],
      "estimates" : [ ],
      "children" : [ {
        "id" : "171",
        "name" : "LocalExchange",
        "descriptor" : {
          "connectorHandleType" : "SystemPartitioningHandle",
          "partitioning" : "SINGLE",
          "function" : "SINGLE",
          "isReplicateNullsAndAny" : "",
          "hashColumn" : "",
          "arguments" : ""
        },
        "outputs" : [ {
          "symbol" : "symbol_717400177",
          "type" : "double"
        } ],
        "details" : [ ],
        "estimates" : [ ],
        "children" : [ {
          "id" : "138",
          "name" : "RemoteSource",
          "descriptor" : {
            "sourceFragmentIds" : "[1]"
          },
          "outputs" : [ {
            "symbol" : "symbol_717400177",
            "type" : "double"
          } ],
          "details" : [ ],
          "estimates" : [ ],
          "children" : [ ]
        } ]
      } ]
    } ]
  },
  "1" : {
    "id" : "137",
    "name" : "LimitPartial",
    "descriptor" : {
      "count" : "10",
      "withTies" : "",
      "inputPreSortedBy" : ""
    },
    "outputs" : [ {
      "symbol" : "symbol_717400177",
      "type" : "double"
    } ],
    "details" : [ ],
    "estimates" : [ ],
    "children" : [ {
      "id" : "0",
      "name" : "TableScan",
      "descriptor" : {
        "connector" : "tpch",
        "table" : "catalog_225088199.schema_225081830.table_3191690477"
      },
      "outputs" : [ {
        "symbol" : "symbol_717400177",
        "type" : "double"
      } ],
      "details" : [ "symbol_717400177 := column_2086283230" ],
      "estimates" : [ ],
      "children" : [ ]
    } ]
  }
}

[sopel39]

this should always exists, right?

[raunaqmorarka]

Ideally yes, but I don't know if we should assume that. In future if catalogs can be removed on the fly, I'm not sure if that assumption would continue to hold.

[sopel39]

In future if catalogs can be removed on the fly, I'm not sure if that assumption would continue to hold.

I don't think they can be removed for active query

[gaurav8297]

this should always exists, right?

I tried making it not optional, but there were many tests which started failing. IIRC these errors were coming in the case of an information schema.

[sopel39]

Ok. Could you tell why for information schema connector name is missing?

[martint]

Why is this an interface? There should only be one implementation of the anonymizer, so an interface is overkill.

[raunaqmorarka]

One implementation performs anonymisation, the other doesn't. Could you take a look at its usage in PlanPrinter and suggest a different way if this can be improved ?

[martint]

What's the use case for including both the regular plan and the anonymized plan? Most event listeners will just forward and store the event as is, which defeats the purpose of having an anonymized plan.

[raunaqmorarka]

It is possible to have different use cases for regular plan and anonymised plan. E.g. Regular plan can be used to display query plan for past queries to a user in a UI. This would be user specific data that might not be made easily accessible to everyone, it's also formatted in a way that is not ideal for offline analysis.
Anonymized plan could be sent to a different downstream system which is more widely accessible and easier to work with for offline analysis.
Currently we send same event to all event listeners, so we also can't use different event listeners to get different versions of plan.

[sopel39]

nit: should it be separate PR?

[gaurav8297]

@raunaqmorarka @sopel39 PTAL

[raunaqmorarka]

lgtm
will wait to see if @martint or @sopel39 have further comments

[sopel39]

Ok. Could you tell why for information schema connector name is missing?

[gaurav8297]

Could you tell why for information schema connector name is missing?

For some reason, We don't register the system or information schema's catalog name to CatalogManager, which is used during listCatalogs. That's why internal catalog names are missing from listCatalogs results.

https://github.com/trinodb/trino/blob/master/core/trino-main/src/main/java/io/trino/connector/ConnectorManager.java#L255

PS: Github is not letting me add a comment in the open thread 😞

cc @sopel39

[sopel39]

lgtm % comments

[sopel39]

why not keep using formatHash method here?

[gaurav8297]

We've made formatHash to be a non-static method

[sopel39]

nit: consider making it non static so that you don't have to pass anonymizer

[gaurav8297]

It'll be redundant to make formatAggregation static because it is being used in GraphVizPlanPrinter too.

[gaurav8297]

Anyways in PlanPrinter, formatAggregation is only used at 2-3 places.

[sopel39]

Is failure related?

[gaurav8297]

@martint @raunaqmorarka @sopel39 Implemented counter-based anonymization

[martint]

What's the purpose of this?

The AST is meant to be a closed hierarchy and not to be extended by third-parties. There's a lot of infrastructure that depends on knowing exactly what classes are part of the AST. Eventually, we'll update it to use Java 17's sealed types and this won't be possible to do at all and enforced at compile time.

[raunaqmorarka]

The idea here is to test the scenario that a new sub-class of Literal is added but CounterBasedAnonymizer#anonymizeExpression isn't updated to handle that new class. In this scenario, we should still anonymise the literal.
It's not strictly necessary though, let me know if you want this removed or handled differently.

[martint]

Yes, let’s remove it. It’s not a correct usage of the AST classes and will break in the future.

As long as the anonymizer fails if a new class is added (very unlikely), we’ll be able to catch that very quickly and add the relevant code, so we don’t even have to handle anonymization for the general case.

[raunaqmorarka]

I've removed this now and modified CounterBasedAnonymizer#anonymizeExpression to throw UnsupportedOperationException in case of un-handled Literal sub-class.

[martint]

I still have concerns about this, which I mentioned before in a related context. Anonymized expressions are not valid SQL, so we should not be trying to construct an AST out of them (effectively what this method does)

[raunaqmorarka]

The API of Anonymizer is String anonymize(Expression expression), so we don't really want an Anonymized expression, an anonymised string representation of the Expression will do.
This method is creating an Expression because it was convenient to use an ExpressionRewriter to anonymise literals and then use Expression#toString on the result.
I think an alternative could be that we write something similar to ExpressionFormatter#Formatter that delegates to existing formatter for all methods except the ones we want to anonymise (visitXXXLiteral).
Would that be better or is there another way that you would recommend instead ?

[martint]

That would be better. Alternatively, we should consider and explore making the expression formatter itself anonymizer-aware.

[gaurav8297]

I've updated the expression formatter such that we could use that directly instead of creating anonymized AST. PTAL @martint

The general approach looks good now. I'll leave it up to @sopel39 to do the final review

[sopel39]

lgtm % comments

[sopel39]

We should anonymize both jsonPlan and plan. Could you create an issue + add TODO?

[gaurav8297]

We are anonymizing both plan and jsonPlan. It's just there's no test for plan in TestEventListenerBasic. In general, there's no good testing of the text plan.

[gaurav8297]

@sopel39 I've addressed comments

[sopel39]

nit: this should mention that both plan and jsonPlan are anonymized