Onboarding calcite opensearch revision publishing setups

[peterzhuamazon]

Description

Onboarding calcite opensearch revision publishing setups

Related Issues

opensearch-project/opensearch-build#5810.
Superseded #21501 and taken patch through that PR.
SQL plugin side: opensearch-project/sql#5302 (sets the TCCL before invoking Calcite).

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 49528d0.

Path	Line	Severity	Description
.github/workflows/calcite-snapshots.yml	32	high	Workflow checks out an arbitrary user-supplied ref from the external apache/calcite repository, builds JARs from it, and uploads those artifacts to a shared S3 bucket. Any actor with workflow_dispatch permission can supply a malicious ref (e.g. a compromised branch) and inject backdoored Calcite JARs into the shared artifact store that downstream OpenSearch builds would consume — a classic supply chain injection vector.
gradle/libs.versions.toml	107	high	New dependency entries added for Apache Calcite (calcite = "1.41.0", calcite_os_rev = "1"). Per mandatory review policy, all dependency additions must be flagged for maintainer verification regardless of apparent legitimacy. Artifact authenticity cannot be confirmed from the diff alone.
.github/workflows/calcite-snapshots.yml	31	medium	All GitHub Actions are pinned to mutable version tags (actions/checkout@v6, aws-actions/configure-aws-credentials@v6, actions/setup-java@v5) rather than immutable commit SHAs. If any of these tags are moved or hijacked, the workflow will silently execute attacker-controlled action code with id-token:write and AWS credential access.
sandbox/patches/calcite/0001-CALCITE-3745-prefer-TCCL-for-Janino-parent-classloader.patch	100	medium	TcclChainedClassLoader makes Janino's compiler consult the Thread Context ClassLoader first before the Calcite-local loader. In OpenSearch's multi-tenant plugin environment, any plugin can set TCCL to its own classloader. A malicious plugin could shadow Calcite internal class names (or any class resolved by name in Janino-compiled query expressions) with its own implementations, enabling code injection into query execution without modifying Calcite itself.
.github/workflows/calcite-snapshots.yml	3	low	Workflow is named 'OpenSearch Lucene snapshots' but its sole purpose is to build and publish Apache Calcite artifacts. This name mismatch could cause maintainers to overlook the workflow during security reviews that specifically audit Calcite-related supply chain processes.

The table above displays the top 10 most important findings.

Total: 5 | Critical: 0 | High: 2 | Medium: 2 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[peterzhuamazon]

Will be accessible once it is published on https://ci.opensearch.org/ci/dbc/snapshots/maven/org/apache/calcite/

[peterzhuamazon]

This is expected addition to snapshots publishing for datafusion.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit e10c599)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incorrect Workflow Name The workflow name is 'OpenSearch Lucene snapshots' but the workflow builds and publishes Calcite snapshots, not Lucene. This mismatch will cause confusion when viewing workflow runs or debugging issues. name: OpenSearch Lucene snapshots Fragile Version Parsing The grep -E commands on lines 59-60 extract version numbers from TOML without validating the file format. If gradle/libs.versions.toml is malformed, missing the expected keys, or has unexpected whitespace, BASE_VER or REV will be empty strings, causing sed to produce an invalid calcite.version= line and the Gradle build to fail with a cryptic error. BASE_VER=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite" | grep -Eo "[0-9]+\.[0-9]+\.[0-9]+"` REV=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite_os_rev" | grep -Eo "[0-9]+"` CALCITE_VER=$BASE_VER-opensearch-$REV sed -i "s/calcite\.version.*/calcite.version=$CALCITE_VER/" gradle.properties Unvalidated Secret Value Line 74 retrieves a secret and assigns it to lucene_snapshots_bucket without checking if the command succeeded or if the value is non-empty. If the secret is missing or the AWS call fails, the variable will be empty, and the final aws s3 cp command will attempt to copy to s3:///snapshots/..., which is an invalid S3 path and will fail. lucene_snapshots_bucket=`aws secretsmanager get-secret-value --secret-id jenkins-artifact-bucket-name --query SecretString --output text` echo "::add-mask::$lucene_snapshots_bucket" echo "LUCENE_SNAPSHOTS_BUCKET=$lucene_snapshots_bucket" >> $GITHUB_OUTPUT

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to e10c599

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Check patch application success The git apply command should verify the patch applies successfully before continuing. If the patch fails to apply due to conflicts or missing context, the build will proceed with unpatched code, producing incorrect artifacts. .github/workflows/calcite-snapshots.yml [58] -git apply os_main/${{ github.event.inputs.patch_file_path }} +git apply os_main/${{ github.event.inputs.patch_file_path }} || { echo "Patch failed to apply"; exit 1; } Suggestion importance[1-10]: 9 __ Why: This is a critical suggestion. If git apply fails silently, the workflow will build and publish unpatched Calcite artifacts, defeating the entire purpose of the workflow. The error handling ensures the build fails fast when the patch cannot be applied.	High
	Validate version extraction success The version extraction commands may fail silently if the patterns don't match, leading to empty variables. Add validation to ensure BASE_VER and REV are non-empty before constructing CALCITE_VER, or the build will proceed with an invalid version string. .github/workflows/calcite-snapshots.yml [59-60] BASE_VER=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite" | grep -Eo "[0-9]+\.[0-9]+\.[0-9]+"` REV=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite_os_rev" | grep -Eo "[0-9]+"` +if [ -z "$BASE_VER" ] || [ -z "$REV" ]; then + echo "Error: Failed to extract version information" + exit 1 + fi Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that BASE_VER and REV extraction could fail silently, leading to an invalid CALCITE_VER string. Adding validation prevents publishing artifacts with malformed version strings, which is a critical issue for dependency management.	Medium
General	Prevent stale TCCL reference The tccl reference is captured in the anonymous ClassLoader subclass, but TCCL can change between when chain() is called and when loadClass() executes. This creates a potential race condition where the wrong classloader is used if the thread's context classloader changes. sandbox/patches/calcite/0001-CALCITE-3745-prefer-TCCL-for-Janino-parent-classloader.patch [128-147] public static ClassLoader chain(ClassLoader fallback) { final ClassLoader tccl = Thread.currentThread().getContextClassLoader(); if (tccl == null || tccl == fallback) { return fallback; } return new ClassLoader(fallback) { @Override protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException { - try { - Class<?> c = tccl.loadClass(name); - ... - } catch (ClassNotFoundException e) { - return super.loadClass(name, resolve); + ClassLoader currentTccl = Thread.currentThread().getContextClassLoader(); + if (currentTccl != null) { + try { + Class<?> c = currentTccl.loadClass(name); + if (resolve) { + resolveClass(c); + } + return c; + } catch (ClassNotFoundException e) { + // Fall through to parent + } } + return super.loadClass(name, resolve); } }; } Suggestion importance[1-10]: 4 __ Why: While the suggestion identifies a theoretical race condition, the captured tccl reference is intentional design. The chain() method creates a classloader that delegates to the TCCL at the time of creation, which is the expected behavior for establishing a stable classloader hierarchy. Re-querying TCCL on every loadClass() call would change the semantics and could introduce instability.	Low

---

Previous suggestions

Suggestions up to commit 16c1353

Category	Suggestion	Impact
General	Fix incorrect workflow name The workflow name references "Lucene snapshots" but the workflow builds and publishes Calcite snapshots. This mismatch could cause confusion when reviewing workflow runs or debugging issues. .github/workflows/calcite-snapshots.yml [3] -name: OpenSearch Lucene snapshots +name: OpenSearch Calcite snapshots Suggestion importance[1-10]: 8 __ Why: The workflow name incorrectly references "Lucene snapshots" when it actually builds and publishes Calcite snapshots. This is a clear naming error that could cause significant confusion during workflow monitoring and debugging.	Medium
	Add version extraction validation The grep patterns may fail if the version format changes or if there are multiple matching lines. Add error handling to verify that BASE_VER and REV are successfully extracted before using them in CALCITE_VER. .github/workflows/calcite-snapshots.yml [54-55] -BASE_VER=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite" | grep -Eo "[0-9]+\.[0-9]+\.[0-9]+"` -REV=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite_os_rev" | grep -Eo "[0-9]+"` +BASE_VER=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite\s*=" | grep -Eo "[0-9]+\.[0-9]+\.[0-9]+"` +REV=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite_os_rev\s*=" | grep -Eo "[0-9]+"` +if [ -z "$BASE_VER" ] || [ -z "$REV" ]; then + echo "Failed to extract version information" + exit 1 +fi Suggestion importance[1-10]: 7 __ Why: The grep patterns could fail silently if the version format changes or if no matches are found, leading to an empty CALCITE_VER variable. Adding validation prevents potential build failures and improves error handling, though the improved regex patterns with \s*= are more robust than the original.	Medium

Suggestions up to commit 03581c7

Category	Suggestion	Impact
Possible issue	Add patch application error handling Add error handling for the git apply command to fail the workflow if the patch cannot be applied. Without this, the build continues with unpatched code, potentially publishing incorrect artifacts. .github/workflows/calcite-snapshots.yml [53] -git apply os_main/sandbox/patches/calcite/0001-CALCITE-3745-prefer-TCCL-for-Janino-parent-classloader.patch +git apply os_main/sandbox/patches/calcite/0001-CALCITE-3745-prefer-TCCL-for-Janino-parent-classloader.patch || { echo "Failed to apply patch"; exit 1; } BASE_VER=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite" | grep -Eo "[0-9]+\.[0-9]+\.[0-9]+"` REV=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite_os_rev" | grep -Eo "[0-9]+"` Suggestion importance[1-10]: 8 __ Why: Adding error handling for git apply is critical to prevent publishing incorrect artifacts if the patch fails to apply. Without this check, the workflow would continue with unpatched code, potentially causing runtime issues.	Medium
	Validate version extraction success Add validation to ensure BASE_VER and REV are successfully extracted before using them. If the grep patterns fail to match, these variables will be empty, resulting in an invalid version string like -opensearch-. .github/workflows/calcite-snapshots.yml [54-55] BASE_VER=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite" | grep -Eo "[0-9]+\.[0-9]+\.[0-9]+"` REV=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite_os_rev" | grep -Eo "[0-9]+"` +if [ -z "$BASE_VER" ] || [ -z "$REV" ]; then echo "Failed to extract version"; exit 1; fi Suggestion importance[1-10]: 8 __ Why: Validating that BASE_VER and REV are successfully extracted prevents publishing artifacts with malformed version strings like -opensearch-. This could cause dependency resolution failures downstream.	Medium
General	Capture TCCL reference safely The anonymous ClassLoader captures tccl which may change after creation if the thread's context classloader is modified. Store tccl as a final field in the anonymous class to ensure consistent behavior throughout the classloader's lifetime. sandbox/patches/calcite/0001-CALCITE-3745-prefer-TCCL-for-Janino-parent-classloader.patch [128-147] public static ClassLoader chain(ClassLoader fallback) { final ClassLoader tccl = Thread.currentThread().getContextClassLoader(); if (tccl == null || tccl == fallback) { return fallback; } + final ClassLoader capturedTccl = tccl; return new ClassLoader(fallback) { @Override protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException { try { - Class<?> c = tccl.loadClass(name); + Class<?> c = capturedTccl.loadClass(name); ... } catch (ClassNotFoundException e) { return super.loadClass(name, resolve); } } }; } Suggestion importance[1-10]: 3 __ Why: While tccl is already declared final and captured by the anonymous class, explicitly renaming it to capturedTccl adds marginal clarity. The existing code is functionally correct since final variables are safely captured in Java closures.	Low

Suggestions up to commit 49528d0

Category	Suggestion	Impact
Possible issue	Add patch application error handling Add error handling for the git apply command to fail the workflow if the patch cannot be applied. Without this check, subsequent commands will execute with unpatched code, potentially publishing incorrect artifacts to S3. .github/workflows/calcite-snapshots.yml [51-58] - name: Apply Patches and build calcite jars run: | - git apply os_main/sandbox/patches/calcite/0001-CALCITE-3745-prefer-TCCL-for-Janino-parent-classloader.patch + git apply os_main/sandbox/patches/calcite/0001-CALCITE-3745-prefer-TCCL-for-Janino-parent-classloader.patch || { echo "Failed to apply patch"; exit 1; } BASE_VER=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite" | grep -Eo "[0-9]+\.[0-9]+\.[0-9]+"` REV=`cat os_main/gradle/libs.versions.toml | grep -E "^calcite_os_rev" | grep -Eo "[0-9]+"` CALCITE_VER=$BASE_VER-opensearch-$REV sed -i "s/calcite\.version.*/calcite.version=$CALCITE_VER/" gradle.properties ./gradlew :core:publishToMavenLocal :linq4j:publishToMavenLocal -Prelease -PskipSign -PskipJavadoc -x test --no-daemon Suggestion importance[1-10]: 7 __ Why: Adding explicit error handling for git apply prevents silent failures that could lead to publishing unpatched artifacts. However, GitHub Actions already fails on non-zero exit codes by default, so this is primarily for clarity and explicit error messaging.	Medium
	Validate S3 bucket retrieval success Add validation to ensure the S3 bucket name is not empty before proceeding. If the secret retrieval fails silently or returns an empty value, the subsequent S3 copy command will fail with a cryptic error or potentially target an incorrect location. .github/workflows/calcite-snapshots.yml [66-71] - name: Get S3 Bucket id: get_s3_bucket run: | lucene_snapshots_bucket=`aws secretsmanager get-secret-value --secret-id jenkins-artifact-bucket-name --query SecretString --output text` + if [ -z "$lucene_snapshots_bucket" ]; then echo "Failed to retrieve S3 bucket name"; exit 1; fi echo "::add-mask::$lucene_snapshots_bucket" echo "LUCENE_SNAPSHOTS_BUCKET=$lucene_snapshots_bucket" >> $GITHUB_OUTPUT Suggestion importance[1-10]: 7 __ Why: Validating that lucene_snapshots_bucket is not empty prevents cryptic failures in the subsequent S3 copy step. This is a reasonable defensive check, though AWS CLI commands typically fail with clear errors if secrets are missing.	Medium

[github-actions]

Persistent review updated to latest commit 03581c7

[github-actions]

Persistent review updated to latest commit 16c1353

[github-actions]

Persistent review updated to latest commit e10c599

[github-actions]

❕ Gradle check result for e10c599: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.51%. Comparing base (90be262) to head (e10c599).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21549      +/-   ##
============================================
+ Coverage     73.43%   73.51%   +0.08%     
- Complexity    74533    74615      +82     
============================================
  Files          5978     5978              
  Lines        338740   338740              
  Branches      48842    48842              
============================================
+ Hits         248748   249023     +275     
+ Misses        70128    69883     -245     
+ Partials      19864    19834      -30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.