All tests working.

This is still missing comments, and we neeed to discuss and understand the ICU Levels etc.
ad-freiburg · Jul 10, 2024 · 1f38f43 · 1f38f43
1 parent e763d98
commit 1f38f43
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 10 deletions.
diff --git a/src/global/ValueId.h b/src/global/ValueId.h
@@ -89,6 +89,12 @@ class ValueId {
 
   // The largest representable integer value.
   static constexpr int64_t maxInt = IntegerType::max();
+  // These types store strings. When sorting IDs, then these types are still
+  // interleaved (meaning that there is a consecutive range of IDs that contains
+  // VocabIndex and LocalVocabIndex, but those two datatypes do not necessarily
+  // form contiguous ranges).
+  static constexpr std::array<Datatype, 2> stringTypes_{
+      Datatype::VocabIndex, Datatype::LocalVocabIndex};
 
   /// This exception is thrown if we try to store a value of an index type
   /// (VocabIndex, LocalVocabIndex, TextRecordIndex) that is larger than

diff --git a/src/global/ValueIdComparators.h b/src/global/ValueIdComparators.h
@@ -16,6 +16,9 @@ namespace valueIdComparators {
 // Equal, NotEqual, GreaterEqual, GreaterThan.
 enum struct Comparison { LT, LE, EQ, NE, GE, GT };
 
+static constexpr std::array stringTypes{Datatype::VocabIndex,
+                                        Datatype::LocalVocabIndex};
+
 inline int orderingToInt(std::strong_ordering o) {
   if (o == std::strong_ordering::less) {
     return -1;
@@ -435,7 +438,11 @@ inline std::vector<std::pair<RandomIt, RandomIt>> getRangesForEqualIds(
   AD_CONTRACT_CHECK(valueIdBegin <= valueIdEnd);
   // This lambda enforces the invariants `non-empty` and `sorted` and also
   // merges directly adjacent ranges.
-  AD_CONTRACT_CHECK(valueIdBegin.getDatatype() == valueIdEnd.getDatatype());
+  auto typeA = valueIdBegin.getDatatype();
+  auto typeB = valueIdEnd.getDatatype();
+  AD_CONTRACT_CHECK(typeA == typeB ||
+                    (ad_utility::contains(stringTypes, typeA) &&
+                     ad_utility::contains(stringTypes, typeB)));
   switch (valueIdBegin.getDatatype()) {
     case Datatype::Double:
     case Datatype::Int:

diff --git a/test/AggregateExpressionTest.cpp b/test/AggregateExpressionTest.cpp
@@ -53,12 +53,11 @@ TEST(AggregateExpression, max) {
   auto testMaxId = testAggregate<MaxExpression, Id>;
   testMaxId({I(3), U, I(0), I(4), U, (I(-1))}, I(4));
   testMaxId({V(7), U, V(2), V(4)}, V(7));
-  testMaxId({I(3), U, V(0), L(3), U, (I(-1))}, L(3));
+  testMaxId({I(3), U, V(0), L(3), U, (I(-1))}, V(0));
 
   auto testMaxString = testAggregate<MaxExpression, IdOrLiteralOrIri>;
-  // TODO<joka921> Implement correct comparison on strings
   testMaxString({lit("alpha"), lit("äpfel"), lit("Beta"), lit("unfug")},
-                lit("äpfel"));
+                lit("unfug"));
 }
 
 // ______________________________________________________________________________
@@ -72,7 +71,7 @@ TEST(AggregateExpression, min) {
   auto testMinString = testAggregate<MinExpression, IdOrLiteralOrIri>;
   // TODO<joka921> Implement correct comparison on strings
   testMinString({lit("alpha"), lit("äpfel"), lit("Beta"), lit("unfug")},
-                lit("Beta"));
+                lit("alpha"));
 }
 
 // ______________________________________________________________________________

diff --git a/test/ValueIdComparatorsTest.cpp b/test/ValueIdComparatorsTest.cpp
@@ -132,9 +132,6 @@ auto testGetRangesForId(auto begin, auto end, ValueId id,
         ++it;
       }
       while (it != rangeEnd) {
-        if (!isMatching(*it, id)) {
-          auto x = isMatching(*it, id);
-        }
         ASSERT_TRUE(isMatching(*it, id)) << *it << ' ' << id << comparison;
         ASSERT_EQ(compareIds(*it, id, comparison), True) << *it << ' ' << id;
         ++it;
@@ -287,6 +284,10 @@ TEST_F(ValueIdComparators, IndexTypes) {
     };
 
     auto applyComparator = [&](auto comparator, ValueId a, ValueId b) {
+      if (a.getDatatype() == Datatype::LocalVocabIndex ||
+          a.getDatatype() == Datatype::VocabIndex) {
+        return comparator(a, b);
+      }
       return comparator(std::invoke(getFromId, a), std::invoke(getFromId, b));
     };
 
@@ -308,9 +309,9 @@ TEST_F(ValueIdComparators, IndexTypes) {
 
   // TODO<joka921> The tests for local vocab and VocabIndex now have to be more
   // complex....
-  // testImpl.operator()<Datatype::VocabIndex>(&getVocabIndex);
+  testImpl.operator()<Datatype::VocabIndex>(&getVocabIndex);
   testImpl.operator()<Datatype::TextRecordIndex>(&getTextRecordIndex);
-  // testImpl.operator()<Datatype::LocalVocabIndex>(&getLocalVocabIndex);
+  testImpl.operator()<Datatype::LocalVocabIndex>(&getLocalVocabIndex);
   testImpl.operator()<Datatype::WordVocabIndex>(&getWordVocabIndex);
 }
 

diff --git a/test/ValueIdTest.cpp b/test/ValueIdTest.cpp
@@ -150,6 +150,12 @@ TEST_F(ValueIdTest, OrderingDifferentDatatypes) {
   std::sort(ids.begin(), ids.end());
 
   auto compareByDatatypeAndIndexTypes = [](ValueId a, ValueId b) {
+    auto typeA = a.getDatatype();
+    auto typeB = b.getDatatype();
+    if (ad_utility::contains(ValueId::stringTypes_, typeA) &&
+        ad_utility::contains(ValueId::stringTypes_, typeB)) {
+      return false;
+    }
     return a.getDatatype() < b.getDatatype();
   };
   ASSERT_TRUE(