Fix bug in TCK row comparison #559
Conversation
There was a problem hiding this comment.
I see what should be achieved and that is indeed an improvement. I am not sure I like the solution. That the equality comparison is so heavily bases on strings seems not right. So, I would rather add a case for CypherPropertyMap (and probably other CypherValues) in line 45 of CypherValue.scala.
But if time is short and you do not want to do it that way, I am not standing in your way. It would be good to add the test for Maps, though. That should be easy too.
| a shouldBe a | ||
| b shouldBe b |
There was a problem hiding this comment.
I know have not touched this bit shouldn't is be:
| a shouldBe a | |
| b shouldBe b | |
| a shouldBe b | |
| b shouldBe a |
otherwise it looks a bit pointless to me or I am missing something.
There was a problem hiding this comment.
We have that below. Since the equals methods are so complex for list values I wanted to test this too. It's easy to implement equals that will return false even if we're comparing the same instance, so it provides some value.
tools/tck-api/src/test/scala/org/opencypher/tools/tck/values/CypherValueTest.scala
Show resolved
Hide resolved
| override def toString: String = s"{${properties.map { | ||
| case (k, v) => s"$k: $v" | ||
| }.mkString(", ")}}" | ||
|
|
||
| override def toString: String = toStringSorted | ||
|
|
||
| // Hack to make ordering work :/ | ||
| private lazy val toStringSorted: String = | ||
| properties.toSeq | ||
| .sortBy { case (k, _) => k } | ||
| .map { case (k, v) => s"$k: $v" } | ||
| .mkString("{", ", ", "}") |
There was a problem hiding this comment.
I am not sure I understand why this fixes the bug.
println(
CypherPropertyMap(Map("a"->CypherInteger(1), "b"->CypherInteger(2)))
==
CypherPropertyMap(Map("b"->CypherInteger(2), "a"->CypherInteger(1)))
)
prints true without this.
If equal comparison falls back to the implicit ordering in line 39 by some Scala magic, which currently escapes ms, then it would be better to fix that to not really on string comparison.
There was a problem hiding this comment.
I agree this situation is not ideal and I too would prefer something more robust. This solves the bug because:
- When comparing results we will compare unique row counts. The row count is a
Map[Map[String, CypherValue], Int], the row as key and count as value in map. - When creating that map we need to calculate the hash code for the rows. The hash code for CypherListValues is done on the sorted elements in the list. It is like this even for ordered lists, because they need to have the same hash code. The ordering in the list is (mostly) based on ordering
toString.
…ypherValueTest.scala Co-authored-by: Hannes Voigt <30618026+hvub@users.noreply.github.com>
Another bug fix uncovered this bug and led to TCK test failures when bumping TCK version in neo4j.