Skip to content

Refactor: convert forPattern if-else chains to switch on FormatNames … - #22568

Open
rajat315315 wants to merge 1 commit into
opensearch-project:mainfrom
rajat315315:refactor/date-format-if-else-to-switch
Open

Refactor: convert forPattern if-else chains to switch on FormatNames …#22568
rajat315315 wants to merge 1 commit into
opensearch-project:mainfrom
rajat315315:refactor/date-format-if-else-to-switch

Conversation

@rajat315315

Copy link
Copy Markdown
Contributor

Fixes: #22567

Description

Replace the 60-70 branch if-else chains in Joda.forPattern() and DateFormatters.forPattern() with switch statements on the FormatNames enum value already resolved by FormatNames.forName(input).

Previously, each method called FormatNames.forName(input) for the camel-case deprecation check (a full linear scan) and then immediately ran a second full linear scan via the if-else chain, calling FormatNames.XXX.matches(input) on every branch (up to 120 string comparisons in the worst case).

The switch dispatch compiles to a JVM tableswitch/lookupswitch instruction (O(1)) and eliminates the redundant second scan entirely. The observable behaviour is identical.

Benefits:

  • O(1) enum dispatch vs O(n) string comparisons on the hot path
  • Compiler exhaustiveness checking: missing FormatNames cases are now detectable at build time via IDE/spotbugs warnings
  • Improved readability: 1-to-1 mapping between FormatNames constant and formatter is immediately visible

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.

…enum

Replace the 60-70 branch if-else chains in Joda.forPattern() and
DateFormatters.forPattern() with switch statements on the FormatNames
enum value already resolved by FormatNames.forName(input).

Previously, each method called FormatNames.forName(input) for the
camel-case deprecation check (a full linear scan) and then immediately
ran a second full linear scan via the if-else chain, calling
FormatNames.XXX.matches(input) on every branch (up to 120 string
comparisons in the worst case).

The switch dispatch compiles to a JVM tableswitch/lookupswitch
instruction (O(1)) and eliminates the redundant second scan entirely.
The observable behaviour is identical.

Benefits:
- O(1) enum dispatch vs O(n) string comparisons on the hot path
- Compiler exhaustiveness checking: missing FormatNames cases are
  now detectable at build time via IDE/spotbugs warnings
- Improved readability: 1-to-1 mapping between FormatNames constant
  and formatter is immediately visible

Signed-off-by: Rajat Jain <rajatjain.ix@gmail.com>
@rajat315315
rajat315315 requested a review from a team as a code owner July 25, 2026 12:22
@github-actions github-actions Bot added bug Something isn't working Search:Performance labels Jul 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

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

Behavior Change

Previously, when the input contained || combined with a recognized format name (e.g. "date||yyyy"), FormatNames.forName(input) would return null (no exact match) and the code fell through to the || split branch. In the new code, FormatNames.forName may still return null for such combined inputs, but for any single-name input that resolves to a format not handled by the switch (falls to default), the code now proceeds to the || split path only if it contains ||, otherwise falls through further. Verify that inputs that previously matched a FormatNames.*.matches(input) check (which historically used exact/camelCase name matching) still resolve identically via FormatNames.forName — any divergence in matching semantics (e.g. handling of whitespace/case) would silently change parsing behavior.

if (formatName != null) {
    switch (formatName) {
        case BASIC_DATE:
            formatter = ISODateTimeFormat.basicDate();
            break;
        case BASIC_DATE_TIME:
            formatter = ISODateTimeFormat.basicDateTime();
            break;
        case BASIC_DATE_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.basicDateTimeNoMillis();
            break;
        case BASIC_ORDINAL_DATE:
            formatter = ISODateTimeFormat.basicOrdinalDate();
            break;
        case BASIC_ORDINAL_DATE_TIME:
            formatter = ISODateTimeFormat.basicOrdinalDateTime();
            break;
        case BASIC_ORDINAL_DATE_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.basicOrdinalDateTimeNoMillis();
            break;
        case BASIC_TIME:
            formatter = ISODateTimeFormat.basicTime();
            break;
        case BASIC_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.basicTimeNoMillis();
            break;
        case BASIC_T_TIME:
            formatter = ISODateTimeFormat.basicTTime();
            break;
        case BASIC_T_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.basicTTimeNoMillis();
            break;
        case BASIC_WEEK_DATE:
            formatter = ISODateTimeFormat.basicWeekDate();
            break;
        case BASIC_WEEK_DATE_TIME:
            formatter = ISODateTimeFormat.basicWeekDateTime();
            break;
        case BASIC_WEEK_DATE_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.basicWeekDateTimeNoMillis();
            break;
        case DATE:
            formatter = ISODateTimeFormat.date();
            break;
        case DATE_HOUR:
            formatter = ISODateTimeFormat.dateHour();
            break;
        case DATE_HOUR_MINUTE:
            formatter = ISODateTimeFormat.dateHourMinute();
            break;
        case DATE_HOUR_MINUTE_SECOND:
            formatter = ISODateTimeFormat.dateHourMinuteSecond();
            break;
        case DATE_HOUR_MINUTE_SECOND_FRACTION:
            formatter = ISODateTimeFormat.dateHourMinuteSecondFraction();
            break;
        case DATE_HOUR_MINUTE_SECOND_MILLIS:
            formatter = ISODateTimeFormat.dateHourMinuteSecondMillis();
            break;
        case DATE_OPTIONAL_TIME:
            // in this case, we have a separate parser and printer since the dataOptionalTimeParser can't print
            // this sucks we should use the root local by default and not be dependent on the node
            return new JodaDateFormatter(
                input,
                ISODateTimeFormat.dateOptionalTimeParser().withLocale(Locale.ROOT).withZone(DateTimeZone.UTC).withDefaultYear(1970),
                ISODateTimeFormat.dateTime().withLocale(Locale.ROOT).withZone(DateTimeZone.UTC).withDefaultYear(1970)
            );
        case DATE_TIME:
            formatter = ISODateTimeFormat.dateTime();
            break;
        case DATE_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.dateTimeNoMillis();
            break;
        case HOUR:
            formatter = ISODateTimeFormat.hour();
            break;
        case HOUR_MINUTE:
            formatter = ISODateTimeFormat.hourMinute();
            break;
        case HOUR_MINUTE_SECOND:
            formatter = ISODateTimeFormat.hourMinuteSecond();
            break;
        case HOUR_MINUTE_SECOND_FRACTION:
            formatter = ISODateTimeFormat.hourMinuteSecondFraction();
            break;
        case HOUR_MINUTE_SECOND_MILLIS:
            formatter = ISODateTimeFormat.hourMinuteSecondMillis();
            break;
        case ORDINAL_DATE:
            formatter = ISODateTimeFormat.ordinalDate();
            break;
        case ORDINAL_DATE_TIME:
            formatter = ISODateTimeFormat.ordinalDateTime();
            break;
        case ORDINAL_DATE_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.ordinalDateTimeNoMillis();
            break;
        case TIME:
            formatter = ISODateTimeFormat.time();
            break;
        case TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.timeNoMillis();
            break;
        case T_TIME:
            formatter = ISODateTimeFormat.tTime();
            break;
        case T_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.tTimeNoMillis();
            break;
        case WEEK_DATE:
            formatter = ISODateTimeFormat.weekDate();
            break;
        case WEEK_DATE_TIME:
            formatter = ISODateTimeFormat.weekDateTime();
            break;
        case WEEK_DATE_TIME_NO_MILLIS:
            formatter = ISODateTimeFormat.weekDateTimeNoMillis();
            break;
        case WEEKYEAR:
            getDeprecationLogger().deprecate(
                "week_year_format_name",
                "Format name \"week_year\" is deprecated and will be removed in a future version. Use \"weekyear\" format instead"
            );
            formatter = ISODateTimeFormat.weekyear();
            break;
        case WEEK_YEAR:
            formatter = ISODateTimeFormat.weekyear();
            break;
        case WEEK_YEAR_WEEK:
            formatter = ISODateTimeFormat.weekyearWeek();
            break;
        case WEEKYEAR_WEEK_DAY:
            formatter = ISODateTimeFormat.weekyearWeekDay();
            break;
        case YEAR:
            formatter = ISODateTimeFormat.year();
            break;
        case YEAR_MONTH:
            formatter = ISODateTimeFormat.yearMonth();
            break;
        case YEAR_MONTH_DAY:
            formatter = ISODateTimeFormat.yearMonthDay();
            break;
        case EPOCH_SECOND:
            formatter = new DateTimeFormatterBuilder().append(new EpochTimePrinter(false), new EpochTimeParser(false))
                .toFormatter();
            break;
        case EPOCH_MILLIS:
            formatter = new DateTimeFormatterBuilder().append(new EpochTimePrinter(true), new EpochTimeParser(true)).toFormatter();
            break;
        // strict date formats here, must be at least 4 digits for year and two for months and two for day
        case STRICT_BASIC_WEEK_DATE:
            formatter = StrictISODateTimeFormat.basicWeekDate();
            break;
        case STRICT_BASIC_WEEK_DATE_TIME:
            formatter = StrictISODateTimeFormat.basicWeekDateTime();
            break;
        case STRICT_BASIC_WEEK_DATE_TIME_NO_MILLIS:
            formatter = StrictISODateTimeFormat.basicWeekDateTimeNoMillis();
            break;
        case STRICT_DATE:
            formatter = StrictISODateTimeFormat.date();
            break;
        case STRICT_DATE_HOUR:
            formatter = StrictISODateTimeFormat.dateHour();
            break;
        case STRICT_DATE_HOUR_MINUTE:
            formatter = StrictISODateTimeFormat.dateHourMinute();
            break;
        case STRICT_DATE_HOUR_MINUTE_SECOND:
            formatter = StrictISODateTimeFormat.dateHourMinuteSecond();
            break;
        case STRICT_DATE_HOUR_MINUTE_SECOND_FRACTION:
            formatter = StrictISODateTimeFormat.dateHourMinuteSecondFraction();
            break;
        case STRICT_DATE_HOUR_MINUTE_SECOND_MILLIS:
            formatter = StrictISODateTimeFormat.dateHourMinuteSecondMillis();
            break;
        case STRICT_DATE_OPTIONAL_TIME:
            // in this case, we have a separate parser and printer since the dataOptionalTimeParser can't print
            // this sucks we should use the root local by default and not be dependent on the node
            return new JodaDateFormatter(
                input,
                StrictISODateTimeFormat.dateOptionalTimeParser()
                    .withLocale(Locale.ROOT)
                    .withZone(DateTimeZone.UTC)
                    .withDefaultYear(1970),
                StrictISODateTimeFormat.dateTime().withLocale(Locale.ROOT).withZone(DateTimeZone.UTC).withDefaultYear(1970)
            );
        case STRICT_DATE_TIME:
            formatter = StrictISODateTimeFormat.dateTime();
            break;
        case STRICT_DATE_TIME_NO_MILLIS:
            formatter = StrictISODateTimeFormat.dateTimeNoMillis();
            break;
        case STRICT_HOUR:
            formatter = StrictISODateTimeFormat.hour();
            break;
        case STRICT_HOUR_MINUTE:
            formatter = StrictISODateTimeFormat.hourMinute();
            break;
        case STRICT_HOUR_MINUTE_SECOND:
            formatter = StrictISODateTimeFormat.hourMinuteSecond();
            break;
        case STRICT_HOUR_MINUTE_SECOND_FRACTION:
            formatter = StrictISODateTimeFormat.hourMinuteSecondFraction();
            break;
        case STRICT_HOUR_MINUTE_SECOND_MILLIS:
            formatter = StrictISODateTimeFormat.hourMinuteSecondMillis();
            break;
        case STRICT_ORDINAL_DATE:
            formatter = StrictISODateTimeFormat.ordinalDate();
            break;
        case STRICT_ORDINAL_DATE_TIME:
            formatter = StrictISODateTimeFormat.ordinalDateTime();
            break;
        case STRICT_ORDINAL_DATE_TIME_NO_MILLIS:
            formatter = StrictISODateTimeFormat.ordinalDateTimeNoMillis();
            break;
        case STRICT_TIME:
            formatter = StrictISODateTimeFormat.time();
            break;
        case STRICT_TIME_NO_MILLIS:
            formatter = StrictISODateTimeFormat.timeNoMillis();
            break;
        case STRICT_T_TIME:
            formatter = StrictISODateTimeFormat.tTime();
            break;
        case STRICT_T_TIME_NO_MILLIS:
            formatter = StrictISODateTimeFormat.tTimeNoMillis();
            break;
        case STRICT_WEEK_DATE:
            formatter = StrictISODateTimeFormat.weekDate();
            break;
        case STRICT_WEEK_DATE_TIME:
            formatter = StrictISODateTimeFormat.weekDateTime();
            break;
        case STRICT_WEEK_DATE_TIME_NO_MILLIS:
            formatter = StrictISODateTimeFormat.weekDateTimeNoMillis();
            break;
        case STRICT_WEEKYEAR:
            formatter = StrictISODateTimeFormat.weekyear();
            break;
        case STRICT_WEEKYEAR_WEEK:
            formatter = StrictISODateTimeFormat.weekyearWeek();
            break;
        case STRICT_WEEKYEAR_WEEK_DAY:
            formatter = StrictISODateTimeFormat.weekyearWeekDay();
            break;
        case STRICT_YEAR:
            formatter = StrictISODateTimeFormat.year();
            break;
        case STRICT_YEAR_MONTH:
            formatter = StrictISODateTimeFormat.yearMonth();
            break;
        case STRICT_YEAR_MONTH_DAY:
            formatter = StrictISODateTimeFormat.yearMonthDay();
            break;
        default:
            // formatName is non-null but has no Joda formatter (e.g. ISO8601, RFC3339_LENIENT, EPOCH_MICROS);
            // fall through to the pattern-based path below.
            formatter = null;
            break;
    }
    if (formatter != null) {
        formatter = formatter.withLocale(Locale.ROOT).withZone(DateTimeZone.UTC).withDefaultYear(1970);
        return new JodaDateFormatter(input, formatter, formatter);
    }
}
Silent Fallback To Pattern Parsing

When formatName != null but hits the default branch (no case matched), execution falls through to new JavaDateFormatter(...appendPattern(input)...), which will try to interpret a known format-name string as a literal DateTimeFormatter pattern. Previously, an unrecognized format name that passed FormatNames.forName but wasn't in the if-else chain would also fall through, but any newly added FormatNames enum constant without a corresponding switch case will now be silently mis-parsed rather than being caught. Consider throwing IllegalArgumentException in the default branch, or logging, to prevent regressions when new enum values are added.

        default:
            break;
    }
}

try {
    return new JavaDateFormatter(
        input,
        new DateTimeFormatterBuilder().appendPattern(input).toFormatter(Locale.ROOT).withResolverStyle(ResolverStyle.STRICT)
    );
} catch (IllegalArgumentException e) {
    throw new IllegalArgumentException("Invalid format: [" + input + "]: " + e.getMessage(), e);
}

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix inverted deprecation warning mapping

The deprecation-warning branch was swapped between WEEKYEAR and WEEK_YEAR compared
to the original if-else chain. Originally, FormatNames.WEEKYEAR.matches(input)
triggered the deprecation warning (because WEEKYEAR's snake_case name is
"week_year"), and WEEK_YEAR did not. Restore the original behavior by moving the
deprecation log to the WEEK_YEAR case (or whichever enum has snake-case
"week_year"), otherwise deprecation semantics for user input are inverted.

server/src/main/java/org/opensearch/common/joda/Joda.java [228-237]

             case WEEKYEAR:
+                formatter = ISODateTimeFormat.weekyear();
+                break;
+            case WEEK_YEAR:
                 getDeprecationLogger().deprecate(
                     "week_year_format_name",
                     "Format name \"week_year\" is deprecated and will be removed in a future version. Use \"weekyear\" format instead"
                 );
                 formatter = ISODateTimeFormat.weekyear();
                 break;
-            case WEEK_YEAR:
-                formatter = ISODateTimeFormat.weekyear();
-                break;
Suggestion importance[1-10]: 8

__

Why: The original code emitted the deprecation warning when FormatNames.WEEKYEAR.matches(input) was true (matching snake-case "week_year"), but the new switch attaches the warning to the WEEKYEAR enum case. Since the enum names in FormatNames may map differently, this could indeed invert the deprecation semantics, which is a legitimate concern worth verifying.

Medium
General
Reject unsupported named formats explicitly

Falling through to the pattern-based path for named formats like ISO8601,
RFC3339_LENIENT, or EPOCH_MICROS will attempt to interpret the name string (e.g.
"iso8601") as a Joda pattern, likely throwing IllegalArgumentException rather than a
meaningful error. Explicitly reject these known-but-unsupported Joda format names
with a clear error message instead of silently treating them as patterns.

server/src/main/java/org/opensearch/common/joda/Joda.java [368-378]

             default:
-                // formatName is non-null but has no Joda formatter (e.g. ISO8601, RFC3339_LENIENT, EPOCH_MICROS);
-                // fall through to the pattern-based path below.
-                formatter = null;
-                break;
+                throw new IllegalArgumentException(
+                    "format name [" + formatName.getSnakeCaseName() + "] is not supported by the Joda formatter"
+                );
         }
-        if (formatter != null) {
-            formatter = formatter.withLocale(Locale.ROOT).withZone(DateTimeZone.UTC).withDefaultYear(1970);
-            return new JodaDateFormatter(input, formatter, formatter);
-        }
+        formatter = formatter.withLocale(Locale.ROOT).withZone(DateTimeZone.UTC).withDefaultYear(1970);
+        return new JodaDateFormatter(input, formatter, formatter);
     }
Suggestion importance[1-10]: 5

__

Why: Providing a clearer error for known-but-unsupported Joda format names (like ISO8601, RFC3339_LENIENT) improves user-facing error clarity, but the fallback to pattern parsing would already throw an exception. This is a moderate quality improvement, though it may also change existing behavior.

Low
Verify deprecation warning attaches to correct enum

The deprecation warning was originally emitted when the input matched
FormatNames.WEEK_YEAR in this file, but ensure the enum-to-warning mapping remains
consistent with the previous if-else logic. Verify that the case triggering the
deprecation corresponds to the same input string(s) as before; if
FormatNames.WEEK_YEAR matched the string "week_year" previously, keep the
deprecation there — otherwise the user-facing deprecation behavior silently changes.

server/src/main/java/org/opensearch/common/time/DateFormatters.java [2100-2108]

+            case WEEK_YEAR:
+                deprecationLogger.getOrCompute()
+                    .deprecate(
+                        "week_year_format_name",
+                        "Format name \"week_year\" is deprecated and will be removed in a future version. Use \"weekyear\" format instead"
+                    );
+                return WEEK_YEAR;
+            case WEEKYEAR:
+                return WEEKYEAR;
 
-
Suggestion importance[1-10]: 4

__

Why: This is a verification-only suggestion asking to double-check the enum-to-warning mapping is consistent, without proposing an actual code change (existing and improved code are identical). Limited actionable value.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Search:Performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] forPattern() performs up to 120 redundant String.equals() comparisons on every date-field request

1 participant