Introduce SparkParameterComposerCollection #2774

ykmr1224 · 2024-06-24T23:46:18Z

Description

Introduce SparkParameterComposerCollection to abstract the datasource type specific logics from async-query-core
DatasourceType specific parameters can be implemented in DataSourceSparkParameterComposer and registered to SparkParameterComposerCollection.
SparkParameterComposerCollection will hold all the implementation of DataSourceSparkParameterComposer and GeneralSparkParameterComposer, and apply them when composeByDatasource and compose is called.
I'm planning to remove SparkSubmitParameterModifier in separate PR since Composer will cover the use case.

Issues Resolved

n/a

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

ykmr1224 · 2024-06-27T22:00:54Z

async-query-core/src/main/java/org/opensearch/sql/spark/parameter/SparkSubmitParameters.java

+import lombok.Setter;
+
+/** Define Spark Submit Parameters. */
+public class SparkSubmitParameters {


Moved from:
async-query-core/src/main/java/org/opensearch/sql/spark/asyncquery/model/SparkSubmitParameters.java
And builder was extracted as separate class.

ykmr1224 · 2024-06-27T22:02:49Z

...uery-core/src/main/java/org/opensearch/sql/spark/parameter/SparkSubmitParametersBuilder.java

+import org.opensearch.sql.spark.dispatcher.model.DispatchQueryRequest;
+import org.opensearch.sql.spark.execution.statestore.OpenSearchStateStoreUtil;
+
+public class SparkSubmitParametersBuilder {


The logic in this builder was originally within SparkSubmitParameters class.
S3GLUE related configs are extracted to S3GlueDataSourceSparkParameterComposer

is it GeneralSparkParameterComposer? Hive and S3 relation configuration should in datasource composer?

...-core/src/main/java/org/opensearch/sql/spark/parameter/DataSourceSparkParameterComposer.java

penghuo · 2024-06-28T16:20:23Z

...-core/src/main/java/org/opensearch/sql/spark/parameter/SparkParameterComposerCollection.java

+/** Stores Spark parameter composers and dispatch compose request to each composer */
+public class SparkParameterComposerCollection {
+  Collection<GeneralSparkParameterComposer> generalComposers = new ArrayList<>();
+  Map<DataSourceType, Collection<DataSourceSparkParameterComposer>> datasourceComposers =


why need map? how about Collection of DataSourceSparkParameterComposer?

We want to lookup by DataSourceType. Do you mean we want to lookup using linear search?

no, i mean why not use Collection of DataSourceSparkParameterComposer.
understood now,

penghuo · 2024-06-28T16:26:02Z

...uery-core/src/main/java/org/opensearch/sql/spark/parameter/SparkSubmitParametersBuilder.java

+import org.opensearch.sql.spark.dispatcher.model.DispatchQueryRequest;
+import org.opensearch.sql.spark.execution.statestore.OpenSearchStateStoreUtil;
+
+public class SparkSubmitParametersBuilder {


is it GeneralSparkParameterComposer? Hive and S3 relation configuration should in datasource composer?

...ain/java/org/opensearch/sql/spark/config/SparkExecutionEngineConfigClusterSettingLoader.java

penghuo · 2024-06-28T16:29:36Z

async-query/src/main/java/org/opensearch/sql/spark/config/OpenSearchExtraParameterComposer.java

+      SparkSubmitParameters sparkSubmitParameters,
+      DispatchQueryRequest dispatchQueryRequest,
+      AsyncQueryRequestContext context) {
+    settingLoader


do we need Setting loader, why not directly access setting in here.

Since it had access from two places, I extracted as a class to avoid redundancy. (It include AccessController.doPrivileged, etc.)

penghuo · 2024-06-28T16:32:27Z

...ry/src/main/java/org/opensearch/sql/spark/config/OpenSearchSparkSubmitParameterModifier.java


 @AllArgsConstructor
 public class OpenSearchSparkSubmitParameterModifier implements SparkSubmitParameterModifier {

  private String extraParameters;

  @Override
-  public void modifyParameters(SparkSubmitParameters parameters) {
-    parameters.setExtraParameters(this.extraParameters);
+  public void modifyParameters(SparkSubmitParametersBuilder builder) {


could u explain more on Modifier and Composer and add to PR description. are these necessary? could we simply the concept?

Added description. I am planning to delete Modifier since Generic composer will cover the use case.

...ry/src/main/java/org/opensearch/sql/spark/config/SparkExecutionEngineConfigSupplierImpl.java

...ery-core/src/main/java/org/opensearch/sql/spark/parameter/GeneralSparkParameterComposer.java

vmmusings · 2024-06-28T18:55:51Z

...c-query-core/src/main/java/org/opensearch/sql/spark/config/SparkSubmitParameterModifier.java


 /**
 * Interface for extension point to allow modification of spark submit parameter. modifyParameter
- * method is called after the default spark submit parameter is build.
+ * method is called after the default spark submit parameter is build. To be deprecated in favor of
+ * {@link org.opensearch.sql.spark.parameter.GeneralSparkParameterComposer}
 */
 public interface SparkSubmitParameterModifier {


When does modifier come into picture?
So we have two concepts composer and modifier. is modifier used after the completion of building spark submit parameters using composer?

I am thinking to remove Modifier once I can verify the Composer can satisfy the use cases.

vmmusings · 2024-06-28T20:36:02Z

Please resolve the conflict.

Signed-off-by: Tomoyuki Morita <[email protected]>

opensearch-trigger-bot · 2024-07-12T22:10:37Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.x
# Create a new branch
git switch --create backport/backport-2774-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a151a7d4484134afb597473871a56a831ffbf323
# Push it to GitHub
git push --set-upstream origin backport/backport-2774-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2774-to-2.x.

* Introduce SparkParameterComposerCollection Signed-off-by: Tomoyuki Morita <[email protected]> * Fix comments Signed-off-by: Tomoyuki Morita <[email protected]> * Fix integ test Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]> (cherry picked from commit a151a7d)

* Introduce SparkParameterComposerCollection Signed-off-by: Tomoyuki Morita <[email protected]> * Fix comments Signed-off-by: Tomoyuki Morita <[email protected]> * Fix integ test Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]>

ykmr1224 force-pushed the dqs/datasource-extension branch from fb6ded6 to 8d16732 Compare June 24, 2024 23:47

ykmr1224 added backport 2.x maintenance Improves code quality, but not the product labels Jun 24, 2024

ykmr1224 force-pushed the dqs/datasource-extension branch from 8d16732 to 607ac20 Compare June 25, 2024 15:28

ykmr1224 marked this pull request as ready for review June 25, 2024 19:34

ykmr1224 requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vmmusings, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto and GumpacG as code owners June 25, 2024 19:34

ykmr1224 commented Jun 27, 2024

View reviewed changes

penghuo reviewed Jun 28, 2024

View reviewed changes

vmmusings reviewed Jun 28, 2024

View reviewed changes

ykmr1224 force-pushed the dqs/datasource-extension branch from 8a77cc1 to 5dac199 Compare June 28, 2024 21:14

penghuo previously approved these changes Jun 28, 2024

View reviewed changes

vmmusings previously approved these changes Jun 28, 2024

View reviewed changes

ykmr1224 dismissed stale reviews from vmmusings and penghuo via f2f0cf5 June 28, 2024 22:52

vmmusings previously approved these changes Jun 29, 2024

View reviewed changes

ykmr1224 dismissed vmmusings’s stale review via fe9b35f June 29, 2024 05:24

ykmr1224 added 3 commits June 28, 2024 22:54

Introduce SparkParameterComposerCollection

0c59d5d

Signed-off-by: Tomoyuki Morita <[email protected]>

Fix comments

eb49d98

Signed-off-by: Tomoyuki Morita <[email protected]>

Fix integ test

9f2162e

Signed-off-by: Tomoyuki Morita <[email protected]>

ykmr1224 force-pushed the dqs/datasource-extension branch from fe9b35f to 9f2162e Compare June 29, 2024 05:59

penghuo approved these changes Jul 1, 2024

View reviewed changes

vmmusings approved these changes Jul 12, 2024

View reviewed changes

ykmr1224 merged commit a151a7d into opensearch-project:main Jul 12, 2024
13 of 20 checks passed

opensearch-trigger-bot bot added the backport-failed label Jul 12, 2024

ykmr1224 mentioned this pull request Jul 12, 2024

[Backport 2.x] Introduce SparkParameterComposerCollection (#2774) #2824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce SparkParameterComposerCollection #2774

Introduce SparkParameterComposerCollection #2774

ykmr1224 commented Jun 24, 2024 •

edited

Loading

ykmr1224 Jun 27, 2024

ykmr1224 Jun 27, 2024

penghuo Jun 28, 2024

penghuo Jun 28, 2024 •

edited

Loading

ykmr1224 Jun 28, 2024

penghuo Jun 28, 2024

penghuo Jun 28, 2024

penghuo Jun 28, 2024

ykmr1224 Jun 28, 2024

penghuo Jun 28, 2024

ykmr1224 Jun 28, 2024

vmmusings Jun 28, 2024 •

edited

Loading

ykmr1224 Jun 28, 2024

vmmusings commented Jun 28, 2024

opensearch-trigger-bot bot commented Jul 12, 2024

Introduce SparkParameterComposerCollection #2774

Introduce SparkParameterComposerCollection #2774

Conversation

ykmr1224 commented Jun 24, 2024 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo Jun 28, 2024 • edited Loading

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

vmmusings Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmmusings commented Jun 28, 2024

opensearch-trigger-bot bot commented Jul 12, 2024

ykmr1224 commented Jun 24, 2024 •

edited

Loading

penghuo Jun 28, 2024 •

edited

Loading

vmmusings Jun 28, 2024 •

edited

Loading