aleksandr-chernousov-db commented on code in PR #58455: URL: https://github.com/apache/spark/pull/58455#discussion_r4006425789
########## common/variant/src/test/scala/org/apache/spark/types/variant/VariantCanonicalizeSuite.scala: ########## @@ -0,0 +1,460 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.types.variant + +import java.util.Arrays + +import org.scalatest.funsuite.AnyFunSuite // scalastyle:ignore funsuite + +/** + * Direct unit tests for `VariantBuilder.canonicalize`. + * + * The canonical form is the contract that `canonicalize(a)` and `canonicalize(b)` are byte-equal + * iff `a` and `b` are semantically equal. It currently covers structural canonicalization + * (metadata dictionary key order, unused-key stripping, object field-id remapping) + * and value normalization for integers, decimals, float/double, and strings + * (integer width, integer-promotion, trailing-zero strip, -0.0 -> +0.0, canonical NaN, short-string + * encoding). The `isCanonical` read-side predicate is complete and checked against `canonicalize` + * by a soundness oracle over a mixed corpus. + */ +class VariantCanonicalizeSuite extends AnyFunSuite { // scalastyle:ignore funsuite + + private def parse(json: String): Variant = + VariantBuilder.parseJson( + json, + /* allowDuplicateKeys = */ false) + + private def canon(v: Variant): Variant = VariantBuilder.canonicalize(v) + + private def bytesEqual(a: Variant, b: Variant): Boolean = + Arrays.equals(a.getValue, b.getValue) && Arrays.equals(a.getMetadata, b.getMetadata) + + private def isCanon(v: Variant): Boolean = + VariantBuilder.isCanonical(v.getValue, v.getMetadata) + + private def buildDouble(d: Double): Variant = { + val b = new VariantBuilder(false) + b.appendDouble(d) + b.result() + } + + private def buildFloat(f: Float): Variant = { + val b = new VariantBuilder(false) + b.appendFloat(f) + b.result() + } + + test("object key order does not affect the canonical form") { + val a = canon(parse("""{"a":1,"b":2}""")) + val b = canon(parse("""{"b":2,"a":1}""")) + assert(bytesEqual(a, b), "objects equal up to key order must canonicalize to equal bytes") + } + + test("nested object key order does not affect the canonical form") { + val a = canon(parse("""{"outer":{"a":1,"b":2},"z":3}""")) + val b = canon(parse("""{"z":3,"outer":{"b":2,"a":1}}""")) + assert(bytesEqual(a, b), "nested object key order must be normalized recursively") + } + + test("canonical form is independent of the incoming metadata dictionary order") { + val a = canon(parse("""{"m":{"a":1},"a":{"b":2}}""")) + val b = canon(parse("""{"a":{"b":2},"m":{"a":1}}""")) + assert(bytesEqual(a, b), "canonical metadata must not depend on incoming dictionary order") + } + + test("object key order inside array elements is normalized, array element order is preserved") { + val a = canon(parse("""[{"a":1,"b":2},{"c":3}]""")) + val b = canon(parse("""[{"b":2,"a":1},{"c":3}]""")) + assert(bytesEqual(a, b), "object key order within array elements must be normalized") + + val c = canon(parse("""[1,2]""")) + val d = canon(parse("""[2,1]""")) + assert(!bytesEqual(c, d), "array element order is significant and must be preserved") + } + + test("canonicalize is idempotent") { + val inputs = Seq( + """{"b":2,"a":1}""", + """{"a":1,"b":2}""", + """{"outer":{"z":1,"a":2},"m":[1,2,3]}""", + "[1,2,3]", + "\"hello\"", + "true", + "null", + "1", + "{}", + "[]") + for (json <- inputs) { + val once = canon(parse(json)) + val twice = canon(once) + assert(bytesEqual(once, twice), s"canonicalize must be idempotent for input $json") + } + } + + test("empty object and empty array canonicalize without error") { + assert(bytesEqual(canon(parse("{}")), canon(parse("{}")))) + assert(bytesEqual(canon(parse("[]")), canon(parse("[]")))) + } + + // ----- Value normalization: DECIMAL ----- + + test("integer-valued decimal canonicalizes to the integer encoding") { + assert(!bytesEqual(parse("1.0"), parse("1")), "1.0 and 1 should differ before canon") + assert(bytesEqual(canon(parse("1.0")), canon(parse("1"))), "1.0 must canonicalize to 1") + assert(bytesEqual(canon(parse("1.000")), canon(parse("1"))), "1.000 must canonicalize to 1") + } + + test("decimal trailing zeros are stripped") { + assert(!bytesEqual(parse("1.50"), parse("1.5")), "1.50 and 1.5 should differ before canon") + assert(bytesEqual(canon(parse("1.50")), canon(parse("1.5"))), "1.50 must canonicalize to 1.5") + assert(bytesEqual(canon(parse("1.500")), canon(parse("1.5"))), "1.500 must canonicalize to 1.5") + } + + test("decimal normalization applies inside nested objects and arrays") { + assert(bytesEqual(canon(parse("""{"a":1.0}""")), canon(parse("""{"a":1}""")))) + assert(bytesEqual( + canon(parse("""{"a":[1.0, 2.50]}""")), + canon(parse("""{"a":[1, 2.5]}""")))) + } + + test("non-integer decimal is not promoted to an integer") { + assert(!bytesEqual(canon(parse("1.5")), canon(parse("1"))), "1.5 must not collapse to 1") + assert(!bytesEqual(canon(parse("1.5")), canon(parse("2"))), "1.5 must not collapse to 2") + } + + test("integer decimal too large for a long is not promoted (stays a decimal)") { + val big = "100000000000000000000" + assert(VariantUtil.getType(parse(big).getValue, 0) == VariantUtil.Type.DECIMAL, + "sanity: 10^20 should parse as a DECIMAL") + val canonBig = canon(parse(big)) + assert(VariantUtil.getType(canonBig.getValue, 0) == VariantUtil.Type.DECIMAL, + "10^20 must remain a DECIMAL, not be wrapped into a long") + val actual = VariantUtil.getDecimal(canonBig.getValue, 0) + assert(actual.compareTo(new java.math.BigDecimal(big)) == 0, "10^20 value must be preserved") + } + + // ----- Value normalization: integer width ----- + + test("non-minimal integer width is reduced to the smallest") { + val int8Header = + ((VariantUtil.INT8 << VariantUtil.BASIC_TYPE_BITS) | VariantUtil.PRIMITIVE).toByte + val int8One = + new Variant(Array[Byte](int8Header, 1, 0, 0, 0, 0, 0, 0, 0), parse("1").getMetadata) + assert(!bytesEqual(int8One, parse("1")), "sanity: INT8(1) and INT1(1) differ before canon") + assert(bytesEqual(canon(int8One), canon(parse("1"))), "INT8(1) must reduce to INT1(1)") + } + + // ----- Value normalization: float / double ----- + + test("negative zero canonicalizes to positive zero") { + assert(!bytesEqual(buildDouble(-0.0d), buildDouble(0.0d)), "sanity: -0.0d and +0.0d differ") + assert(bytesEqual(canon(buildDouble(-0.0d)), canon(buildDouble(0.0d))), "double -0.0 -> +0.0") + assert(!bytesEqual(buildFloat(-0.0f), buildFloat(0.0f)), "sanity: -0.0f and +0.0f differ") + assert(bytesEqual(canon(buildFloat(-0.0f)), canon(buildFloat(0.0f))), "float -0.0 -> +0.0") + } + + test("non-canonical NaN canonicalizes to the canonical NaN") { + val doubleHeader = + ((VariantUtil.DOUBLE << VariantUtil.BASIC_TYPE_BITS) | VariantUtil.PRIMITIVE).toByte + val bytes = java.nio.ByteBuffer.allocate(9).order(java.nio.ByteOrder.LITTLE_ENDIAN) + .put(doubleHeader).putLong(0x7ff8000000000001L).array() + val nonCanonicalNaN = new Variant(bytes, parse("1").getMetadata) + assert(!bytesEqual(nonCanonicalNaN, buildDouble(Double.NaN)), "sanity: NaN encodings differ") + assert(bytesEqual(canon(nonCanonicalNaN), canon(buildDouble(Double.NaN))), + "all NaN bit patterns must canonicalize to the same bytes") + } + + // ----- Value normalization: string encoding ----- + + test("a short string stored as long_str is re-encoded as a short string") { + val longStrHeader = + ((VariantUtil.LONG_STR << VariantUtil.BASIC_TYPE_BITS) | VariantUtil.PRIMITIVE).toByte + val text = "hi".getBytes(java.nio.charset.StandardCharsets.UTF_8) + val bytes = java.nio.ByteBuffer.allocate(1 + 4 + text.length) + .order(java.nio.ByteOrder.LITTLE_ENDIAN) + .put(longStrHeader).putInt(text.length).put(text).array() + val longEncoded = new Variant(bytes, parse("1").getMetadata) + assert(!bytesEqual(longEncoded, parse("\"hi\"")), "sanity: long_str and short_str 'hi' differ") + assert(bytesEqual(canon(longEncoded), parse("\"hi\"")), "long_str 'hi' -> short_str") + } + + // ----- isCanonical: metadata dictionary ----- + + test("isCanonical accepts a sorted metadata dictionary and rejects an unsorted one") { + assert(isCanon(parse("""{"a":1,"b":2}""")), "ascending dictionary is canonical") + assert(!isCanon(parse("""{"b":2,"a":1}""")), "descending dictionary is not canonical") + assert(!isCanon(parse("""{"z":1,"a":2,"m":3}""")), "unsorted dictionary is not canonical") + assert(isCanon(canon(parse("""{"b":2,"a":1}"""))), "canon output has a sorted dictionary") + } + + test("isCanonical accepts an empty dictionary") { + assert(isCanon(parse("1")), "a scalar's empty dictionary is canonical") + assert(isCanon(parse("[1,2,3]")), "an array of scalars has an empty dictionary") + } + + test("isCanonical rejects a non-minimal metadata offset width") { + val emptyMeta = parse("1").getMetadata + val version = emptyMeta(0) + val header2 = (version | (1 << 6)).toByte // offset width = 2 bytes + val meta = java.nio.ByteBuffer.allocate(1 + 2 + 2 + 2 + 1) + .order(java.nio.ByteOrder.LITTLE_ENDIAN) + .put(header2) + .put(1.toByte).put(0.toByte) // numKeys = 1 + .put(0.toByte).put(0.toByte) // offset[0] = 0 + .put(1.toByte).put(0.toByte) // offset[1] = 1 (one key byte) + .put('a'.toByte) // key "a" + .array() + // isCanonical inspects only the metadata at this step, so any value bytes suffice. + assert(!VariantBuilder.isCanonical(parse("1").getValue, meta), + "a 2-byte offset width where 1 byte fits is not canonical") + } + + // ----- isCanonical: object / array structure ----- + + test("isCanonical rejects a dictionary with an unused key") { Review Comment: Added tests -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
