From 5a00560ed5bdf85173c057b794604896097308cb Mon Sep 17 00:00:00 2001
From: John Ed Quinn <johqunn@amazon.com>
Date: Mon, 19 Aug 2024 13:16:33 -0700
Subject: [PATCH] Addresses PR comments

---
 .../java/org/partiql/eval/value/Datum.java    | 26 +++++++++++++++++--
 .../partiql/eval/value/DatumComparator.java   | 24 +++++++++++++----
 .../kotlin/org/partiql/value/PartiQLValue.kt  |  1 +
 3 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/partiql-spi/src/main/java/org/partiql/eval/value/Datum.java b/partiql-spi/src/main/java/org/partiql/eval/value/Datum.java
index 0ccf49fea9..0334cc3c9b 100644
--- a/partiql-spi/src/main/java/org/partiql/eval/value/Datum.java
+++ b/partiql-spi/src/main/java/org/partiql/eval/value/Datum.java
@@ -655,8 +655,19 @@ static Datum struct(@NotNull Iterable<Field> values) {
     }
 
     /**
-     * Comparator for PartiQL's scalar comparison operator.
+     * Comparator for PartiQL values.
+     * <p>
+     * This may be used for the comparison operators, GROUP BY, ORDER BY, and DISTINCT. The conventional use
+     * of {@link java.util.HashMap}, {@link java.util.HashSet}, {@link Object#hashCode()}, and
+     * {@link Object#equals(Object)} will not work outright with Datum to implement the before-mentioned operations due
+     * to requirements by the PartiQL and SQL Specifications. One may use {@link java.util.TreeMap} and
+     * {@link java.util.TreeSet} in combination with this {@link Comparator} to implement the before-mentioned
+     * operations.
+     * </p>
      * @return the default comparator for {@link Datum}. The comparator orders null values first.
+     * @see Datum
+     * @see java.util.TreeSet
+     * @see java.util.TreeMap
      */
     @NotNull
     static Comparator<Datum> comparator() {
@@ -664,9 +675,20 @@ static Comparator<Datum> comparator() {
     }
 
     /**
-     * Comparator for PartiQL's scalar comparison operator.
+     * Comparator for PartiQL values.
+     * <p>
+     * This may be used for the comparison operators, GROUP BY, ORDER BY, and DISTINCT. The conventional use
+     * of {@link java.util.HashMap}, {@link java.util.HashSet}, {@link Object#hashCode()}, and
+     * {@link Object#equals(Object)} will not work outright with Datum to implement the before-mentioned operations due
+     * to requirements by the PartiQL and SQL Specifications. One may use {@link java.util.TreeMap} and
+     * {@link java.util.TreeSet} in combination with this {@link Comparator} to implement the before-mentioned
+     * operations.
+     * </p>
      * @param nullsFirst if true, nulls are ordered before non-null values, otherwise after.
      * @return the default comparator for {@link Datum}.
+     * @see Datum
+     * @see java.util.TreeSet
+     * @see java.util.TreeMap
      */
     @NotNull
     static Comparator<Datum> comparator(boolean nullsFirst) {
diff --git a/partiql-spi/src/main/java/org/partiql/eval/value/DatumComparator.java b/partiql-spi/src/main/java/org/partiql/eval/value/DatumComparator.java
index 2ca5cdfed6..1618672103 100644
--- a/partiql-spi/src/main/java/org/partiql/eval/value/DatumComparator.java
+++ b/partiql-spi/src/main/java/org/partiql/eval/value/DatumComparator.java
@@ -34,11 +34,13 @@ abstract class DatumComparator implements Comparator<Datum> {
     private static final int TYPE_KINDS_LENGTH = TYPE_KINDS.length;
 
     /**
-     * This array defines the precedence of types when comparing values of different types.
-     * The lower the index, the higher the precedence. However, this is used for generating the BASELINE precedence.
-     * This may be overridden by families of types. For example, all number values are compared by their mathematical
-     * representation (not their {@link PType.Kind}. Therefore, when comparing two numbers of different types, this
-     * precedence array will not be used.
+     * This array defines the precedence of type families when comparing values of different types. The lower the index,
+     * the higher the precedence. Please see
+     * <a href="https://partiql.org/partiql-lang/#sec:order-by-less-than">PartiQL Specification Section 12.2</a> for
+     * more information.
+     * <p>
+     * This is only used for aiding in the initialization of the {@link #COMPARISON_TABLE}.
+     * </p>
      */
     @NotNull
     private static final Map<PType.Kind, Integer> TYPE_PRECEDENCE = initializeTypePrecedence();
@@ -180,6 +182,18 @@ private static Map<PType.Kind, Integer> initializeTypePrecedence() {
         return precedence;
     }
 
+    /**
+     * This essentially operates in two passes.
+     * <ol>
+     * <li>Initialize the comparisons for the cartesian product of all type kinds based solely on
+     * the {@link #TYPE_PRECEDENCE}.</li>
+     * <li>Rewrite the comparisons where the values themselves will need to be compared. For example, all PartiQL numbers
+     * need to have their JVM primitives extracted before making comparison judgements.</li>
+     * </ol>
+     * @return the 2D comparison table
+     * @see #initializeComparatorArray(PType.Kind)
+     * @see #fillIntComparator(DatumComparison[])
+     */
     @SuppressWarnings("deprecation")
     private static DatumComparison[][] initializeComparators() {
         // Initialize Table
diff --git a/partiql-spi/src/main/kotlin/org/partiql/value/PartiQLValue.kt b/partiql-spi/src/main/kotlin/org/partiql/value/PartiQLValue.kt
index 8048b342f6..f1784526b6 100644
--- a/partiql-spi/src/main/kotlin/org/partiql/value/PartiQLValue.kt
+++ b/partiql-spi/src/main/kotlin/org/partiql/value/PartiQLValue.kt
@@ -85,6 +85,7 @@ public sealed interface PartiQLValue {
          */
         @JvmStatic
         @JvmOverloads
+        @Deprecated("This will be removed in a future major-version release.")
         public fun comparator(nullsFirst: Boolean = true): Comparator<PartiQLValue> = PartiQLValueComparatorInternal(nullsFirst)
     }
 }