PR comments

Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>

Author: vinaykpud

sandbox/libs/analytics-api/src/main/java/org/opensearch/analytics/schema/DateFormatClassifier.java

@@ -28,6 +28,9 @@ public enum Kind {
         TIMESTAMP
     }
 
+    // TODO: replace with constants from OpenSearch core's DateFormatters registry once they're
+    // exposed for external use; today we mirror the well-known names locally to avoid coupling
+    // to internal APIs.
     /** Named formats with year/month/day only. */
     private static final Set<String> NAMED_DATE_FORMATS = setOf(
         "basic_date",

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/CastTemporalLiteralValidator.java

@@ -18,6 +18,7 @@
 import org.apache.calcite.sql.type.SqlTypeName;
 
 /** Pre-isthmus pass that rejects malformed CAST(string-lit AS DATE/TIME/TIMESTAMP) and TIMESTAMPADD/DIFF string args. */
+// TODO: fold into BackendPlanAdaption alongside CastToVarcharRewriter (unified rejection/coercion model).
 final class CastTemporalLiteralValidator {
 
     private CastTemporalLiteralValidator() {}

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ConvertTzAdapter.java

@@ -25,7 +25,9 @@
 import org.opensearch.analytics.spi.ScalarFunctionAdapter;
 
 import java.time.DateTimeException;
+import java.time.LocalDateTime;
 import java.time.ZoneId;
+import java.time.format.DateTimeParseException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Locale;
@@ -79,9 +81,11 @@ private static RexNode foldInvalidTimestampLiteralToNull(
         String value = literal.getValueAs(String.class);
         if (value == null) return null;
         try {
-            java.time.LocalDateTime.parse(value.replace(' ', 'T'));
+            LocalDateTime.parse(value.replace(' ', 'T'));
             return null;
-        } catch (java.time.format.DateTimeParseException ignored) {}
+        } catch (DateTimeParseException ignored) {
+            // unparseable timestamp literal -> NULL row (in line with sql opensearch plugin contract; see ConvertTZFunctionIT)
+        }
         return rexBuilder.makeNullLiteral(resultType);
     }
 

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java

@@ -456,6 +456,8 @@ public byte[] attachFragmentOnTop(RelNode fragment, byte[] innerBytes) {
     }
 
     private byte[] convertToSubstrait(RelNode fragment) {
+        // TODO: move rewriters that don't touch substrait-specific classes up to the analytics-engine
+        // layer so other backends can reuse them.
         RelNode preprocessed = UntypedNullPreprocessor.rewrite(fragment);
         preprocessed = PplAggregateCallRewriter.rewrite(preprocessed);
         preprocessed = PplWindowCallRewriter.rewrite(preprocessed);

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DatetimeLiteralValidator.java

@@ -18,6 +18,7 @@
 import java.util.Locale;
 
 /** Plan-time validation for string-literal datetime operands; rejects malformed input with the format-hint message. */
+// TODO: move this validation into the analytics-engine Marking phase so it runs once across the whole plan with full operator context.
 final class DatetimeLiteralValidator {
 
     private DatetimeLiteralValidator() {}