Adjust shadowJar name by mengweieric · Pull Request #5203 · opensearch-project/sql · GitHub
Skip to content

Adjust shadowJar name#5203

Merged
mengweieric merged 2 commits into
opensearch-project:2.19from
mengweieric:2.19
Mar 5, 2026
Merged

Adjust shadowJar name#5203
mengweieric merged 2 commits into
opensearch-project:2.19from
mengweieric:2.19

Conversation

@mengweieric

@mengweieric mengweieric commented Mar 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Description

Adjust shadowJar name to accommodate change in core opensearch-project/OpenSearch#20767

Fixed all CI build and test failures.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

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.

@mengweieric
Adjust shadowJar name
9e0de60 
Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
ps48
ps48 previously approved these changes Mar 4, 2026
View reviewed changes
@github-actions

github-actions Bot commented Mar 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Mar 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to bca100a

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use explicit WeekFields for deterministic week calculation

The "w" pattern in DateTimeFormatter with Locale.ENGLISH uses the locale's default
week definition, which may not consistently match the EXTRACT function's behavior
across all JVM implementations or locales. Consider using an explicit WeekFields
configuration (e.g., WeekFields.ISO or WeekFields.of(Locale.ENGLISH)) to make the
week calculation deterministic and clearly documented.

core/src/test/java/org/opensearch/sql/expression/datetime/ExtractTest.java [103-104]

-long expectedWeek =
-    Long.parseLong(DateTimeFormatter.ofPattern("w", Locale.ENGLISH).format(now.atStartOfDay()));
+long expectedWeek = now.get(java.time.temporal.WeekFields.of(Locale.ENGLISH).weekOfWeekBasedYear());
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about locale-dependent week calculation, but the PR's intent is specifically to match the EXTRACT function's behavior using DateTimeFormatter.ofPattern("w", Locale.ENGLISH). Switching to WeekFields.of(Locale.ENGLISH).weekOfWeekBasedYear() may not produce the same result as the formatter-based approach, potentially defeating the purpose of the fix.

Low

Previous suggestions

Suggestions up to commit 9de9544
CategorySuggestion                                                                                                                                    Impact
General
Use explicit WeekFields for deterministic week calculation

The "w" pattern in DateTimeFormatter with Locale.ENGLISH uses the locale's week
definition, which may still vary by JVM version or platform. To ensure consistent
behavior, explicitly use a WeekFields-based formatter tied to a fixed locale, or
directly use WeekFields.of(Locale.ENGLISH).weekOfYear() to compute the week number
deterministically.

core/src/test/java/org/opensearch/sql/expression/datetime/ExtractTest.java [103-104]

-long expectedWeek =
-    Long.parseLong(DateTimeFormatter.ofPattern("w", Locale.ENGLISH).format(now.atStartOfDay()));
+long expectedWeek = now.get(java.time.temporal.WeekFields.of(Locale.ENGLISH).weekOfYear());
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a valid concern about locale-dependent week calculation consistency, but the PR's approach using DateTimeFormatter.ofPattern("w", Locale.ENGLISH) is intentionally chosen to match the EXTRACT function's behavior. Using WeekFields.of(Locale.ENGLISH).weekOfYear() may or may not produce the same result as the EXTRACT function, making this suggestion potentially counterproductive to the test's goal of matching production behavior.

Low
Suggestions up to commit dfb903e
CategorySuggestion                                                                                                                                    Impact
General
Ensure locale week definition matches EXTRACT function

The "w" pattern in DateTimeFormatter with Locale.ENGLISH uses the locale's default
week definition, which may not consistently match the behavior of the EXTRACT
function being tested. If the EXTRACT function uses a specific week definition
(e.g., ISO 8601 or US locale), the locale used here should explicitly match that
definition to avoid flaky tests. Consider using WeekFields explicitly or verifying
that Locale.ENGLISH week numbering matches the SQL EXTRACT implementation.

core/src/test/java/org/opensearch/sql/expression/datetime/ExtractTest.java [103-104]

 long expectedWeek =
-    Long.parseLong(DateTimeFormatter.ofPattern("w", Locale.ENGLISH).format(now.atStartOfDay()));
+    Long.parseLong(DateTimeFormatter.ofPattern("w", Locale.ROOT).format(now.atStartOfDay()));
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about locale consistency between the test and the EXTRACT implementation. However, the PR comment explicitly states that Locale.ENGLISH was chosen to match the EXTRACT function's behavior, and changing to Locale.ROOT without verifying the EXTRACT implementation could introduce the very mismatch the PR is trying to fix. The suggestion is speculative rather than definitively correct.

Low
Suggestions up to commit 0b1f2eb
CategorySuggestion                                                                                                                                    Impact
General
Use explicit WeekFields for deterministic week calculation

The "w" pattern in DateTimeFormatter with Locale.ENGLISH uses the locale's default
week definition, which may not consistently match the EXTRACT function's behavior
across all JVM implementations or locales. Consider using an explicit WeekFields
configuration (e.g., WeekFields.ISO or WeekFields.of(Locale.ENGLISH)) to make the
week-of-year calculation deterministic and independent of locale defaults.

core/src/test/java/org/opensearch/sql/expression/datetime/ExtractTest.java [103-104]

-long expectedWeek =
-    Long.parseLong(DateTimeFormatter.ofPattern("w", Locale.ENGLISH).format(now.atStartOfDay()));
+long expectedWeek = now.get(java.time.temporal.WeekFields.of(Locale.ENGLISH).weekOfYear());
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about locale-dependent week calculation, but the PR explicitly chose DateTimeFormatter.ofPattern("w", Locale.ENGLISH) to match the EXTRACT function's behavior. The improved_code using WeekFields.of(Locale.ENGLISH).weekOfYear() may not actually match the EXTRACT function's internal calculation, potentially reintroducing the flakiness the PR is trying to fix.

Low
Suggestions up to commit 0b1f2eb
CategorySuggestion                                                                                                                                    Impact
General
Use explicit WeekFields for deterministic week calculation

The "w" pattern in DateTimeFormatter with Locale.ENGLISH uses the locale's default
week definition, which may not consistently match the EXTRACT function's behavior
across all JVM implementations or locales. Consider using WeekFields explicitly
(e.g., WeekFields.of(Locale.ENGLISH).weekOfWeekBasedYear()) to make the week
calculation deterministic and clearly documented.

core/src/test/java/org/opensearch/sql/expression/datetime/ExtractTest.java [103-104]

-long expectedWeek =
-    Long.parseLong(DateTimeFormatter.ofPattern("w", Locale.ENGLISH).format(now.atStartOfDay()));
+long expectedWeek = now.get(WeekFields.of(Locale.ENGLISH).weekOfYear());
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about locale-dependent week calculation, but the PR's approach using DateTimeFormatter.ofPattern("w", Locale.ENGLISH) is intentionally chosen to match the EXTRACT function's behavior. The improved_code uses WeekFields.of(Locale.ENGLISH).weekOfYear() which may not actually match the EXTRACT function's implementation any better than the current approach, making this a marginal improvement at best.

Low
Suggestions up to commit cc6f809
CategorySuggestion                                                                                                                                    Impact
General
Use explicit week fields for deterministic calculation

The "w" pattern in DateTimeFormatter with Locale.ENGLISH uses the locale's default
week definition, which may not consistently match the EXTRACT function's behavior
across all JVM implementations or locales. Consider using an explicit WeekFields
configuration (e.g., WeekFields.ISO) to ensure deterministic week-of-year
calculation that matches the EXTRACT function's expected behavior.

core/src/test/java/org/opensearch/sql/expression/datetime/ExtractTest.java [103-105]

-long expectedWeek =
-    Long.parseLong(
-        DateTimeFormatter.ofPattern("w", Locale.ENGLISH).format(now.atStartOfDay()));
+long expectedWeek = now.get(java.time.temporal.WeekFields.ISO.weekOfWeekBasedYear());
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a valid concern about locale-dependent week calculation, but the PR's approach using DateTimeFormatter.ofPattern("w", Locale.ENGLISH) was specifically chosen to match the EXTRACT function's behavior. The improved_code using WeekFields.ISO.weekOfWeekBasedYear() is essentially the same approach that was previously removed (similar to ALIGNED_WEEK_OF_YEAR) and may not actually match the EXTRACT function's implementation better than the proposed fix.

Low

noCharger
noCharger previously approved these changes Mar 5, 2026
View reviewed changes
@github-actions

github-actions Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 0b1f2eb.

PathLineSeverityDescription
async-query-core/build.gradle12lowShadow plugin changed from 'com.github.johnrengelman.shadow' to 'com.gradleup.shadow'. This is the documented official successor maintained by the Gradle community after the original maintainer transferred ownership, but any dependency change should be verified against the official Gradle Plugin Portal to confirm the artifact identity and integrity.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | 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.

@github-actions

github-actions Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0b1f2eb

1 similar comment
@github-actions

github-actions Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0b1f2eb

LantaoJin
LantaoJin reviewed Mar 5, 2026
View reviewed changes
Comment thread .github/workflows/sql-test-and-build-workflow.yml Outdated
@github-actions

github-actions Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit dfb903e

@github-actions

github-actions Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 9de9544

@mengweieric
Fix flaky ExtractTest and update CI macOS runner
bca100a 
- Fix ExtractTest.testExtractDatePartWithTimeType() flaky WEEK assertion
  by using the same DateTimeFormatter("w", Locale.ENGLISH) calculation
  as the EXTRACT function instead of ALIGNED_WEEK_OF_YEAR
- Update CI macOS runner from deprecated macos-13 to macos-14

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
@github-actions

github-actions Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

@mengweieric mengweieric enabled auto-merge (squash) March 5, 2026 06:48
@mengweieric mengweieric merged commit ffa813e into opensearch-project:2.19 Mar 5, 2026
36 of 38 checks passed
@mengweieric mengweieric mentioned this pull request Mar 10, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@penghuo penghuo penghuo approved these changes
@dai-chen dai-chen dai-chen approved these changes
@kavithacm kavithacm Awaiting requested review from kavithacm
@derek-ho derek-ho Awaiting requested review from derek-ho
@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner
@YANG-DB YANG-DB Awaiting requested review from YANG-DB
@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner
@seankao-az seankao-az Awaiting requested review from seankao-az
@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz
@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand
@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner
@forestmvey forestmvey Awaiting requested review from forestmvey
@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner
@GumpacG GumpacG Awaiting requested review from GumpacG
@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner
@LantaoJin LantaoJin Awaiting requested review from LantaoJin
@noCharger noCharger Awaiting requested review from noCharger
@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner
+1 more reviewer
@andrross andrross andrross approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mengweieric @LantaoJin @penghuo @ps48 @andrross @noCharger @dai-chen