[#7206] improvement(javadoc): Fix Javadoc warnings in Trino connector module #7317

KyleLin0927 · 2025-06-01T17:14:00Z

What changes were proposed in this pull request?

Fix the Javadoc in Trino connector module.

Why are the changes needed?

Fixes #7206

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Ran ./gradlew trino-connector:trino-connector:javadoc to confirm the Javadoc warning was resolved.
Ran ./gradlew clean build to ensure the build passes without errors.

KyleLin0927 · 2025-06-01T17:16:45Z

Hi, @justinmclean ,

about #7206, It might be because the Trino connector is compiled using JDK 17, which results in stricter Javadoc checks compared to the Spark connector and other Trino connectors.

As a result, almost all Trino connector modules have added Javadoc comments to their code. If you prefer not to make major modifications to the files, we might need to explore alternative adjustments. I’ve submitted a draft for your reference, if everything looks good to you, I’ll go ahead and create a formal PR.

KyleLin0927 · 2025-06-09T04:57:41Z

@justinmclean I have released the PR, plz take a look, thanks.

jerryshao · 2025-06-09T20:53:59Z

@yuqi1129 @diqiu50 can you please help to review this PR?

justinmclean · 2025-06-10T04:30:51Z

...ctor/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java

  public static final String GRAVITINO_DYNAMIC_CONNECTOR = "__gravitino.dynamic.connector";
+  /** The Gravitino dynamic connector catalog config. */
  public static final String GRAVITINO_DYNAMIC_CONNECTOR_CATALOG_CONFIG =
      "__gravitino.dynamic.connector.catalog.config";


Same with this set of comments above.

I’ve checked these are indeed required to prevent warning messages.

justinmclean · 2025-06-10T04:30:59Z

...ctor/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java

  public static final String TRINO_CATALOG_STORE = "catalog.store";
+  /** The Trino catalog management. */
  public static final String TRINO_CATALOG_MANAGEMENT = "catalog.management";


The above comments are not really needed. There is little point in adding a comment that merely reproduces what the code states.

However, if they are required by Javadocs to remove warnings, then that's okay - the same applies to my other comments below.

I’ve double-checked. these Javadocs are indeed required to prevent warning messages.

justinmclean · 2025-06-10T04:33:35Z

...ctor/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java

    final String defaultValue;
+
+    /** Whether this configuration parameter is required */
    final boolean isRequired;


Again, I'm not sure these comments add any value. Also, why "configuration parameter" when the class is not called that? The distinction between a parameter and an entry may be subtle, but it is nonetheless distinct.

I’ve checked these annotations and found they are unnecessary, so I’ll remove them.”

justinmclean · 2025-06-10T04:36:47Z

...ctor/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorPluginManager.java

+   *
+   * @param connectorName the name of the connector
+   * @param config the config of the connector
+   * @param context the context of the connector


These comments could be improved a little. i.e config -> configuration, and I'm not sure "of the connector" works for the second two parameters.

justinmclean · 2025-06-10T04:37:49Z

...r/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoErrorCode.java

  GRAVITINO_DUPLICATED_CATALOGS(24, EXTERNAL),
+  /** Expression error. */
  GRAVITINO_EXPRESSION_ERROR(25, EXTERNAL);


These comments add little value and make the code harder to read and maintain.

those comments help remove the warnings.

justinmclean · 2025-06-10T04:43:23Z

.../main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java

  protected final List<PropertyMetadata<?>> columnProperties;

+  /** The data type transformer used to convert between Gravitino and Trino types. */
  protected final GeneralDataTypeTransformer dataTypeTransformer;


I'd remove the comments above they don't really add any value.

I’ve double-checked. these Javadocs are indeed required to prevent warning messages.

justinmclean · 2025-06-10T04:47:45Z

...ain/java/org/apache/gravitino/trino/connector/catalog/hive/HiveCatalogPropertyConverter.java

@@ -28,6 +28,7 @@
 /** Convert Apache Hive properties between Trino and Apache Gravitino. */
 public class HiveCatalogPropertyConverter extends PropertyConverter {

+  /** Logger for the HiveCatalogPropertyConverter class. */
  public static final Logger LOG = LoggerFactory.getLogger(HiveCatalogPropertyConverter.class);


There is no need for this comment or, in fact, any similar comments. Comments should a) describe why design decisions were taken b) explain complex code or c) exist to generate Javadocs.

those comments are required for removing the warning, and I will modify the description.

Make the LOG private and remove the comments

justinmclean · 2025-06-10T04:49:27Z

...ector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/ExpressionUtil.java

+   *
+   * @param transforms the array of transforms
+   * @return the list of partition fields
+   */


While this is correct, it might be better to mention expressions here?

justinmclean · 2025-06-10T04:49:37Z

...ector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/ExpressionUtil.java

+   *
+   * @param partitions the list of partition fields
+   * @return the array of transforms
+   */


While this is correct, it might be better to mention expressions here?

justinmclean · 2025-06-10T04:51:08Z

...ector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/ExpressionUtil.java

+   *
+   * @param orderFields the list of sort order fields
+   * @return the array of sort orders
+   */


This doesn't seem to agree with the name of the function. Is the function poorly named or do the comments need updating?

I think the issue is that the function name is not precise. If needed, I can open another PR to fix the name and its unit tests, or if you prefer, I can make the changes directly in this PR.

justinmclean · 2025-06-10T05:01:18Z

@KyleLin0927 This looks good to me. There are a couple of minor comments/things to fix. I had thought at first that you may have added extra unnecessary comments in a couple of places, but it may be that the Javadocs warnings are generated there, if so, please ignore/those comments of mine.

KyleLin0927 · 2025-06-11T04:17:13Z

@KyleLin0927 This looks good to me. There are a couple of minor comments/things to fix. I had thought at first that you may have added extra unnecessary comments in a couple of places, but it may be that the Javadocs warnings are generated there, if so, please ignore/those comments of mine.

thanks for your review, I will double check your comment later.

yuqi1129 · 2025-06-13T12:42:53Z

...-connector/src/main/java/org/apache/gravitino/trino/connector/metadata/GravitinoCatalog.java

+   *
+   * @param region the region to check
+   * @return true if the catalog is in the same region as the specified region, false otherwise
+   */


This may not be related to this PR

public boolean isSameRegion(String region) { // When the Gravitino connector has not configured the cloud.region-code, // or the catalog has not configured the cloud.region-code, // or the catalog cluster name is equal to the connector-configured region code, // the catalog is belong to the region return StringUtils.isEmpty(region) || StringUtils.isEmpty(getRegion()) || region.equals(getRegion()); }

Judging from the code above, it seems that as long as the parameter region is not empty, the result is always true. @diqiu50 Please help to confirm this point.

That’s incorrect — the result is always true even when the region parameter is empty.

yuqi1129 · 2025-06-13T12:47:45Z

...no-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/HasPropertyMeta.java

@@ -26,21 +26,38 @@
 /** Interface for adding property metadata to the catalogs */
 public interface HasPropertyMeta {

-  /** Property metadata for schema */
+  /**
+   * Gets the property metadata for schemas.


Get the property metadata for a specific schema.

KyleLin0927 · 2025-06-19T03:35:08Z

@justinmclean Thank you for reviewing. I’ve double-checked the mentioned code, removed unnecessary logic, and updated the wording.

diqiu50 · 2025-06-20T07:10:16Z

...c/main/java/org/apache/gravitino/trino/connector/catalog/DefaultCatalogConnectorFactory.java

  protected final HashMap<String, CatalogConnectorContext.Builder> catalogBuilders =
      new HashMap<>();
+
+  /** The region code where this catalog connector factory is running */


The region code indicates the location of this Trino instance.

diqiu50

LGTM

justinmclean

LGTM thanks for doing this

…nector module (apache#7317) ### What changes were proposed in this pull request? Fix the Javadoc in Trino connector module. ### Why are the changes needed? Fixes apache#7206 ### Does this PR introduce any user-facing change? No. ### How was this patch tested? - Ran `./gradlew trino-connector:trino-connector:javadoc` to confirm the Javadoc warning was resolved. - Ran `./gradlew clean build` to ensure the build passes without errors.

KyleLin0927 added 2 commits June 2, 2025 00:50

add javadoc in trino connector

6a8902c

Merge branch 'main' into issue-7206

20ec348

xunliu assigned KyleLin0927 Jun 4, 2025

KyleLin0927 marked this pull request as ready for review June 9, 2025 04:56

jerryshao requested review from yuqi1129 and diqiu50 and removed request for yuqi1129 June 9, 2025 20:53

justinmclean reviewed Jun 10, 2025

View reviewed changes

yuqi1129 reviewed Jun 13, 2025

View reviewed changes

improve the changes

19ec17c

diqiu50 reviewed Jun 20, 2025

View reviewed changes

modify the javadoc

753ff07

diqiu50 reviewed Jun 26, 2025

View reviewed changes

justinmclean approved these changes Jun 26, 2025

View reviewed changes

justinmclean merged commit 1cc02e6 into apache:main Jun 26, 2025
27 checks passed

[#7206] improvement(javadoc): Fix Javadoc warnings in Trino connector module #7317

[#7206] improvement(javadoc): Fix Javadoc warnings in Trino connector module #7317

Uh oh!

Conversation

KyleLin0927 commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

KyleLin0927 commented Jun 1, 2025

Uh oh!

KyleLin0927 commented Jun 9, 2025

Uh oh!

jerryshao commented Jun 9, 2025

Uh oh!

justinmclean Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmclean Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diqiu50 Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmclean commented Jun 10, 2025

Uh oh!

KyleLin0927 commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KyleLin0927 commented Jun 19, 2025

KyleLin0927 commented Jun 1, 2025 •

edited

Loading

justinmclean Jun 10, 2025 •

edited

Loading

justinmclean Jun 10, 2025 •

edited

Loading

diqiu50 Jun 20, 2025 •

edited

Loading