apply review suggestions

wraymo · wraymo · commit e9a9140703ff · 2025-04-05T01:37:54.000Z
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java
@@ -33,6 +33,7 @@
 import java.util.Optional;
 
 import static com.facebook.presto.spi.ConnectorPlanRewriter.rewriteWith;
+import static java.util.Objects.requireNonNull;
 
 public class ClpPlanOptimizer
         implements ConnectorPlanOptimizer
@@ -44,8 +45,8 @@ public class ClpPlanOptimizer
     public ClpPlanOptimizer(FunctionMetadataManager functionManager,
                             StandardFunctionResolution functionResolution)
     {
-        this.functionManager = functionManager;
-        this.functionResolution = functionResolution;
+        this.functionManager = requireNonNull(functionManager, "functionManager is null");
+        this.functionResolution = requireNonNull(functionResolution, "functionResolution is null");
     }
 
     @Override
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.java
@@ -34,16 +34,16 @@ public class ClpSplit
 {
     private final SchemaTableName schemaTableName;
     private final String archivePath;
-    private final Optional<String> query;
+    private final Optional<String> kqlQuery;
 
     @JsonCreator
     public ClpSplit(@JsonProperty("schemaTableName") @Nullable SchemaTableName schemaTableName,
                     @JsonProperty("archivePath") @Nullable String archivePath,
-                    @JsonProperty("query") Optional<String> query)
+                    @JsonProperty("kqlQuery") Optional<String> kqlQuery)
     {
         this.schemaTableName = schemaTableName;
         this.archivePath = archivePath;
-        this.query = query;
+        this.kqlQuery = kqlQuery;
     }
 
     @JsonProperty
@@ -60,9 +60,9 @@ public String getArchivePath()
     }
 
     @JsonProperty
-    public Optional<String> getQuery()
+    public Optional<String> getKqlQuery()
     {
-        return query;
+        return kqlQuery;
     }
 
     @Override
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplitManager.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplitManager.java
@@ -23,6 +23,8 @@
 
 import javax.inject.Inject;
 
+import static java.util.Objects.requireNonNull;
+
 public class ClpSplitManager
         implements ConnectorSplitManager
 {
@@ -31,7 +33,7 @@ public class ClpSplitManager
     @Inject
     public ClpSplitManager(ClpSplitProvider clpSplitProvider)
     {
-        this.clpSplitProvider = clpSplitProvider;
+        this.clpSplitProvider = requireNonNull(clpSplitProvider, "clpSplitProvider is null");
     }
 
     @Override
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpTableLayoutHandle.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpTableLayoutHandle.java
@@ -24,14 +24,14 @@ public class ClpTableLayoutHandle
         implements ConnectorTableLayoutHandle
 {
     private final ClpTableHandle table;
-    private final Optional<String> query;
+    private final Optional<String> kqlQuery;
 
     @JsonCreator
     public ClpTableLayoutHandle(@JsonProperty("table") ClpTableHandle table,
-                                @JsonProperty("query") Optional<String> query)
+                                @JsonProperty("query") Optional<String> kqlQuery)
     {
         this.table = table;
-        this.query = query;
+        this.kqlQuery = kqlQuery;
     }
 
     @JsonProperty
@@ -41,9 +41,9 @@ public ClpTableHandle getTable()
     }
 
     @JsonProperty
-    public Optional<String> getQuery()
+    public Optional<String> getKqlQuery()
     {
-        return query;
+        return kqlQuery;
     }
 
     @Override
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java
@@ -95,7 +95,7 @@ public List<ClpSplit> listSplits(ClpTableLayoutHandle clpTableLayoutHandle)
                 while (resultSet.next()) {
                     final String archiveId = resultSet.getString("archive_id");
                     final String archivePath = tablePath + "/" + archiveId;
-                    splits.add(new ClpSplit(tableSchemaName, archivePath, clpTableLayoutHandle.getQuery()));
+                    splits.add(new ClpSplit(tableSchemaName, archivePath, clpTableLayoutHandle.getKqlQuery()));
                 }
             }
         }
diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpSplit.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpSplit.java
@@ -133,7 +133,7 @@ public void testListSplits()
             assertEquals(splits.size(), NUM_SPLITS);
             for (int i = 0; i < NUM_SPLITS; i++) {
                 assertEquals(splits.get(i).getArchivePath(), "/tmp/archives/" + tableName + "/id_" + i);
-                assertEquals(splits.get(i).getQuery(), Optional.empty());
+                assertEquals(splits.get(i).getKqlQuery(), Optional.empty());
             }
         }
     }
diff --git a/presto-docs/src/main/sphinx/connector/clp.rst b/presto-docs/src/main/sphinx/connector/clp.rst
@@ -1,6 +1,6 @@
-=======================
+=============
 CLP Connector
-=======================
+=============
 
 .. contents::
     :local:
@@ -10,7 +10,7 @@ CLP Connector
 Overview
 --------
 
-The CLP Connector enables SQL-based querying of CLP-S archives from Presto. This document describes how to setup the
+The CLP Connector enables SQL-based querying of CLP-S archives from Presto. This document describes how to set up the
 CLP Connector to run SQL queries.
 
 
@@ -39,116 +39,63 @@ Configuration Properties
 
 The following configuration properties are available:
 
-============================================= ==============================================================================
-Property Name                                 Description
-============================================= ==============================================================================
-``clp.archive-source``                        The source of the CLP archive.
-``clp.metadata-expire-interval``              The time interval after which metadata entries are considered expired.
-``clp.metadata-refresh-interval``             The frequency at which metadata is refreshed from the source.
-``clp.polymorphic-type-enabled``              Enables or disables support for polymorphic types within CLP.
-``clp.metadata-source``                       The source from which metadata is fetched.
-``clp.metadata-db-url``                       The connection URL for the metadata database.
-``clp.metadata-db-name``                      The name of the metadata database.
-``clp.metadata-db-user``                      The database user with access to the metadata database.
-``clp.metadata-db-password``                  The password for the metadata database user.
-``clp.metadata-table-prefix``                 A prefix applied to table names in the metadata database.
-``clp.split-source``                          The source of split information for query execution.
-============================================= ==============================================================================
+================================== ======================================================================== =========
+Property Name                      Description                                                              Default
+================================== ======================================================================== =========
+``clp.archive-source``             Specifies the source of the CLP archive. Supported values include        ``local``
+                                   ``local`` (local storage) and ``s3`` (Amazon S3).
+``clp.metadata-expire-interval``   Defines how long metadata entries remain valid before being considered   600
+                                   expired, in seconds.
+``clp.metadata-refresh-interval``  Specifies how frequently metadata is refreshed from the source, in       60
+                                   seconds. Set this to a lower value for frequently changing datasets or
+                                   to a higher value to reduce load.
+``clp.polymorphic-type-enabled``   Enables or disables support for polymorphic types in CLP, allowing the   ``false``
+                                   same field to have different types. This is useful for schema-less,
+                                   semi-structured data where the same field may appear with different
+                                   types.
+
+                                   When enabled, type annotations are added to conflicting field names to
+                                   distinguish between types. For example, if ``id`` column appears as both
+                                   an ``int`` and ``string`` types, the connector will create two columns
+                                   named ``id_bigint`` and ``id_varchar``.
+
+                                   Supported type annotations include ``bigint``, ``varchar``, ``double``,
+                                   ``boolean``, and ``array(varchar)`` (See `Data Types`_ for details). For
+                                   columns with only one type, the original column name is used.
+``clp.metadata-provider-type``     Specifies the metadata provider type. Currently, the only supported      ``mysql``
+                                   type is a MySQL database, which is also used by the CLP package to store
+                                   metadata. Additional providers can be supported by implementing the
+                                   ``ClpMetadataProvider`` interface.
+``clp.metadata-db-url``            The JDBC URL used to connect to the metadata database. This property is
+                                   required if ``clp.metadata-source`` is set to ``mysql``.
+``clp.metadata-db-name``           The name of the metadata database. This option is required if
+                                   ``clp.metadata-source`` is set to ``mysql`` and the database name is not
+                                   specified in the URL.
+``clp.metadata-db-user``           The database user with access to the metadata database.This option is
+                                   required if ``clp.metadata-source`` is set to ``mysql`` and the database
+                                   name is not specified in the URL.
+``clp.metadata-db-password``       The password for the metadata database user. This option is required if
+                                   ``clp.metadata-source`` is set to ``mysql``.
+``clp.metadata-table-prefix``      A string prefix prepended to all metadata table names when querying the
+                                   database. Useful for namespacing or avoiding collisions. This option is
+                                   required if ``clp.metadata-source`` is set to ``mysql``.
+``clp.split-provider-type``        Specifies the split provider type. By default, it uses the same type as  ``mysql``
+                                   the metadata provider with the same connection parameters. Additional
+                                   types can be supported by implementing the ``ClpSplitProvider``
+                                   interface.
+================================== ======================================================================== =========
 
-``clp.archive-source``
-^^^^^^^^^^^^^^^^^^^^^^
 
-Specifies the source of the CLP archive. Supported values include ``local`` (local storage) and ``s3`` (Amazon S3).
-
-This property is optional. The default is ``local``.
-
-``clp.metadata-expire-interval``
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Defines how long metadata entries remain valid before being considered expired, in seconds.
-
-This property is optional. The default is ``600``.
-
-``clp.metadata-refresh-interval``
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Specifies how frequently metadata is refreshed from the source, in seconds. This ensures that metadata remains up to
-date.
-
-Set this to a lower value for frequently changing datasets or to a higher value to reduce load.
-
-This property is optional. The default is ``60``.
-
-``clp.polymorphic-type-enabled``
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Enables or disables support for polymorphic types in CLP, allowing the same field to have different types.
-This is useful for schema-less, semi-structured data where the same field may appear with different types.
-When enabled, type annotations are added to conflicting field names to distinguish between types. For example, if ``id``
-column appears as both an ``int`` and ``string`` types, the connector will create two columns named ``id_bigint`` and
-``id_varchar``.
-
-Supported type annotations include ``bigint``, ``varchar``, ``double``, ``boolean``, and
-``array(varchar)`` (See `Data Types`_ for details). For columns with only one type, the original column name is used.
-
-This property is optional. The default is ``false``.
-
-``clp.metadata-source``
-^^^^^^^^^^^^^^^^^^^^^^^
-Currently, the only supported source is a MySQL database, which is also used by the CLP package to store metadata.
-Additional sources can be supported by implementing the ``ClpMetadataProvider`` interface.
-
-This property is optional. The default is ``mysql``.
-
-``clp.metadata-db-url``
-^^^^^^^^^^^^^^^^^^^^^^^
-The JDBC URL used to connect to the metadata database.
-
-This property is required if ``clp.metadata-source`` is set to ``mysql``.
-
-``clp.metadata-db-name``
-^^^^^^^^^^^^^^^^^^^^^^^^
-
-The name of the metadata database.
-
-This option is required if ``clp.metadata-source`` is set to ``mysql`` and the database name is not specified in the URL.
-
-``clp.metadata-db-user``
-^^^^^^^^^^^^^^^^^^^^^^^^
-
-The username used to authenticate with the metadata database.
-
-Ensure this user has read access to the relevant metadata tables.
-
-This option is required if ``clp.metadata-source`` is set to ``mysql``.
-
-``clp.metadata-db-password``
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-The password for the user specified in ``clp.metadata-db-user``.
-
-This option is required if ``clp.metadata-source`` is set to ``mysql``.
-
-``clp.metadata-table-prefix``
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-A string prefix prepended to all metadata table names when querying the database. Useful for namespacing or avoiding
-collisions.
-
-This option is optional. The default is empty.
-
-``clp.split-source``
-^^^^^^^^^^^^^^^^^^^^
-
-Specifies the source of split information for tables. By default, it uses the same source as the metadata with the same
-connection parameters. Additional sources can be supported by implementing the ``ClpSplitProvider`` interface.
-
-This property is optional. The default is ``mysql``.
 
 Metadata and Split Providers
 ----------------------------
-As mentioned earlier, the CLP connector relies on metadata and split providers to retrieve information from various
-sources. By default, it uses a MySQL database for both metadata and split storage. We recommend using the CLP package
-for log ingestion, which automatically populates the database with the required information. However, if you prefer to
-use a different source—or the same source with a custom implementation—you can provide your own implementations of
-the ``ClpMetadataProvider`` and ``ClpSplitProvider`` interfaces, and configure the connector accordingly.
+The CLP connector relies on metadata and split providers to retrieve information from various sources. By default, it
+uses a MySQL database for both metadata and split storage. We recommend using the CLP package for log ingestion, which
+automatically populates the database with the required information.
+
+If you prefer to use a different source—or the same source with a custom implementation—you can provide your own
+implementations of the ``ClpMetadataProvider`` and ``ClpSplitProvider`` interfaces, and configure the connector
+accordingly.
 
 Data Types
 ----------
@@ -172,7 +119,7 @@ CLP Type               Presto Type
 String Types
 ^^^^^^^^^^^^
 
-In CLP, we have three distinct string types: ``ClpString`` (strings with whitespace), ``VarString`` (strings without
+CLP uses three distinct string types: ``ClpString`` (strings with whitespace), ``VarString`` (strings without
 whitespace), and ``DateString`` (strings representing dates). Currently, all three are mapped to Presto's ``VARCHAR``
 type.
 
@@ -181,13 +128,13 @@ Array Types
 
 CLP supports two array types: ``UnstructuredArray`` and ``StructuredArray``. Unstructured arrays are stored as strings
 in CLP and elements can be any type. However, in Presto arrays are homogeneous, so the elements are converted to strings
-when read. ``StructuredArray`` type is not supported yet.
+when read. ``StructuredArray`` type is not supported in Presto.
 
 Object Types
 ^^^^^^^^^^^^
 CLP stores metadata using a global schema tree structure that captures all possible fields from various log structures.
 Internal nodes may represent objects containing nested fields as their children. In Presto, we map these internal object
-nodes specifically to the ``ROW`` data type, including all subfields as fields within the ``ROW``.
+nodes to the ``ROW`` data type, including all subfields as fields within the ``ROW``.
 
 For instance, consider a table containing two distinct JSON log types:
 
@@ -223,9 +170,9 @@ following ``ROW`` type:
 
    ROW(ts BIGINT, status VARCHAR, thread_num BIGINT, backtrace VARCHAR)
 
-Each JSON log maps to this unified ``ROW`` type, with absent fields represented as ``NULL``. Thus, the child nodes
-(``ts``, ``status``, ``thread_num``, ``backtrace``) become fields within the ``ROW``, clearly reflecting the nested and
-varying structures of the original JSON logs.
+Each JSON log maps to this unified ``ROW`` type, with absent fields represented as ``NULL``. The child nodes (``ts``,
+``status``, ``thread_num``, ``backtrace``) become fields within the ``ROW``, clearly reflecting the nested and varying
+structures of the original JSON logs.
 
 SQL support
 -----------

Original file line number	Diff line number	Diff line change
`@@ -34,16 +34,16 @@ public class ClpSplit`
`34`	`34`	`{`
`35`	`35`	`private final SchemaTableName schemaTableName;`
`36`	`36`	`private final String archivePath;`
`37`		`- private final Optional<String> query;`
	`37`	`+ private final Optional<String> kqlQuery;`
`38`	`38`
`39`	`39`	`@JsonCreator`
`40`	`40`	`public ClpSplit(@JsonProperty("schemaTableName") @Nullable SchemaTableName schemaTableName,`
`41`	`41`	`@JsonProperty("archivePath") @Nullable String archivePath,`
`42`		`- @JsonProperty("query") Optional<String> query)`
	`42`	`+ @JsonProperty("kqlQuery") Optional<String> kqlQuery)`
`43`	`43`	`{`
`44`	`44`	`this.schemaTableName = schemaTableName;`
`45`	`45`	`this.archivePath = archivePath;`
`46`		`- this.query = query;`
	`46`	`+ this.kqlQuery = kqlQuery;`
`47`	`47`	`}`
`48`	`48`
`49`	`49`	`@JsonProperty`
`@@ -60,9 +60,9 @@ public String getArchivePath()`
`60`	`60`	`}`
`61`	`61`
`62`	`62`	`@JsonProperty`
`63`		`- public Optional<String> getQuery()`
	`63`	`+ public Optional<String> getKqlQuery()`
`64`	`64`	`{`
`65`		`- return query;`
	`65`	`+ return kqlQuery;`
`66`	`66`	`}`
`67`	`67`
`68`	`68`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -23,6 +23,8 @@`
`23`	`23`
`24`	`24`	`import javax.inject.Inject;`
`25`	`25`
	`26`	`+import static java.util.Objects.requireNonNull;`
	`27`	`+`
`26`	`28`	`public class ClpSplitManager`
`27`	`29`	`implements ConnectorSplitManager`
`28`	`30`	`{`
`@@ -31,7 +33,7 @@ public class ClpSplitManager`
`31`	`33`	`@Inject`
`32`	`34`	`public ClpSplitManager(ClpSplitProvider clpSplitProvider)`
`33`	`35`	`{`
`34`		`- this.clpSplitProvider = clpSplitProvider;`
	`36`	`+ this.clpSplitProvider = requireNonNull(clpSplitProvider, "clpSplitProvider is null");`
`35`	`37`	`}`
`36`	`38`
`37`	`39`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -24,14 +24,14 @@ public class ClpTableLayoutHandle`
`24`	`24`	`implements ConnectorTableLayoutHandle`
`25`	`25`	`{`
`26`	`26`	`private final ClpTableHandle table;`
`27`		`- private final Optional<String> query;`
	`27`	`+ private final Optional<String> kqlQuery;`
`28`	`28`
`29`	`29`	`@JsonCreator`
`30`	`30`	`public ClpTableLayoutHandle(@JsonProperty("table") ClpTableHandle table,`
`31`		`- @JsonProperty("query") Optional<String> query)`
	`31`	`+ @JsonProperty("query") Optional<String> kqlQuery)`
`32`	`32`	`{`
`33`	`33`	`this.table = table;`
`34`		`- this.query = query;`
	`34`	`+ this.kqlQuery = kqlQuery;`
`35`	`35`	`}`
`36`	`36`
`37`	`37`	`@JsonProperty`
`@@ -41,9 +41,9 @@ public ClpTableHandle getTable()`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`@JsonProperty`
`44`		`- public Optional<String> getQuery()`
	`44`	`+ public Optional<String> getKqlQuery()`
`45`	`45`	`{`
`46`		`- return query;`
	`46`	`+ return kqlQuery;`
`47`	`47`	`}`
`48`	`48`
`49`	`49`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ public List<ClpSplit> listSplits(ClpTableLayoutHandle clpTableLayoutHandle)`
`95`	`95`	`while (resultSet.next()) {`
`96`	`96`	`final String archiveId = resultSet.getString("archive_id");`
`97`	`97`	`final String archivePath = tablePath + "/" + archiveId;`
`98`		`- splits.add(new ClpSplit(tableSchemaName, archivePath, clpTableLayoutHandle.getQuery()));`
	`98`	`+ splits.add(new ClpSplit(tableSchemaName, archivePath, clpTableLayoutHandle.getKqlQuery()));`
`99`	`99`	`}`
`100`	`100`	`}`
`101`	`101`	`}`
Original file line number	Diff line number	Diff line change
`@@ -133,7 +133,7 @@ public void testListSplits()`
`133`	`133`	`assertEquals(splits.size(), NUM_SPLITS);`
`134`	`134`	`for (int i = 0; i < NUM_SPLITS; i++) {`
`135`	`135`	`assertEquals(splits.get(i).getArchivePath(), "/tmp/archives/" + tableName + "/id_" + i);`
`136`		`- assertEquals(splits.get(i).getQuery(), Optional.empty());`
	`136`	`+ assertEquals(splits.get(i).getKqlQuery(), Optional.empty());`
`137`	`137`	`}`
`138`	`138`	`}`
`139`	`139`	`}`