flyrain commented on code in PR #1487:
URL: https://github.com/apache/polaris/pull/1487#discussion_r2071051077
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -90,41 +91,59 @@ public void executeScript(String scriptFilePath) throws
SQLException {
}
/**
- * Executes SELECT Query
+ * Executes SELECT Query and returns the results after applying a transformer
*
* @param query : Query to executed
- * @param entityClass : Class of the entity being selected
- * @param transformer : Transformation of entity class to Result class
Review Comment:
Minor: the method still has a transformer parameter.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -90,41 +91,59 @@ public void executeScript(String scriptFilePath) throws
SQLException {
}
/**
- * Executes SELECT Query
+ * Executes SELECT Query and returns the results after applying a transformer
*
* @param query : Query to executed
- * @param entityClass : Class of the entity being selected
- * @param transformer : Transformation of entity class to Result class
- * @param entityFilter : Filter to applied on the Result class
- * @param limit : Limit to to enforced.
- * @return List of Result class objects
+ * @param converterInstance : An entity of the type being selected, used to
convert to
+ * PolarisBaseEntity
+ * @return The list of results yielded by the query
* @param <T> : Entity class
* @param <R> : Result class
Review Comment:
The term entity feels a bit overloaded here, it doesn’t just refer to
`PolarisBaseEntity`, but also to its sibling types. Would it make sense to
clarify by adopting terms like persistence entity for the database model and
business entity for the service-layer representation? I get that this might
make the <T>, <R> less generic, but it could improve readability. Not a
blocker, just a suggestion.
```suggestion
* @param converterInstance : An persistence entity of the type being
selected, used to convert to
* a business entity like PolarisBaseEntity
* @return The list of results yielded by the query
* @param <T> : Persistence Entity class
* @param <R> : Business Entity class
```
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -90,41 +91,59 @@ public void executeScript(String scriptFilePath) throws
SQLException {
}
/**
- * Executes SELECT Query
+ * Executes SELECT Query and returns the results after applying a transformer
*
* @param query : Query to executed
- * @param entityClass : Class of the entity being selected
- * @param transformer : Transformation of entity class to Result class
- * @param entityFilter : Filter to applied on the Result class
- * @param limit : Limit to to enforced.
- * @return List of Result class objects
+ * @param converterInstance : An entity of the type being selected, used to
convert to
+ * PolarisBaseEntity
+ * @return The list of results yielded by the query
* @param <T> : Entity class
* @param <R> : Result class
* @throws SQLException : Exception during the query execution.
*/
public <T, R> List<R> executeSelect(
@Nonnull String query,
- @Nonnull Class<T> entityClass,
- @Nonnull Function<T, R> transformer,
- Predicate<R> entityFilter,
- int limit)
+ @Nonnull Converter<T> converterInstance,
+ @Nonnull Function<T, R> transformer)
Review Comment:
It seems that we could construct a "result entity" directly from the
`ResultSet`. There is no transformer is required, and converter will need to
take one more type mark like `Converter<T, R>`. Not a blocker though. We could
make that change later.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -90,41 +91,59 @@ public void executeScript(String scriptFilePath) throws
SQLException {
}
/**
- * Executes SELECT Query
+ * Executes SELECT Query and returns the results after applying a transformer
*
* @param query : Query to executed
- * @param entityClass : Class of the entity being selected
- * @param transformer : Transformation of entity class to Result class
- * @param entityFilter : Filter to applied on the Result class
- * @param limit : Limit to to enforced.
- * @return List of Result class objects
+ * @param converterInstance : An entity of the type being selected, used to
convert to
+ * PolarisBaseEntity
+ * @return The list of results yielded by the query
* @param <T> : Entity class
* @param <R> : Result class
* @throws SQLException : Exception during the query execution.
*/
public <T, R> List<R> executeSelect(
@Nonnull String query,
- @Nonnull Class<T> entityClass,
- @Nonnull Function<T, R> transformer,
- Predicate<R> entityFilter,
- int limit)
+ @Nonnull Converter<T> converterInstance,
+ @Nonnull Function<T, R> transformer)
+ throws SQLException {
+ ArrayList<R> results = new ArrayList<>();
+ executeSelectOverStream(
+ query,
+ converterInstance,
+ stream -> {
+ stream.map(transformer).forEach(results::add);
+ });
Review Comment:
Minor style suggestion:
```suggestion
stream -> stream.map(transformer).forEach(results::add)
);
```
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
