PrimitiveType.union erases original type

[asfimport]

The following ScalaTest test case

  test("merge primitive types") {
    val expected =
      Types.buildMessage()
        .addField(
          Types
            .required(INT32)
            .as(DECIMAL)
            .precision(7)
            .scale(2)
            .named("f"))
        .named("root")

    assert(expected.union(expected) === expected)
  }

produces the following assertion error

message root {
  required int32 f;
}
 did not equal message root {
  required int32 f (DECIMAL(9,0));
}

This is because PrimitiveType.union doesn't handle original type properly. An open question is that, can two primitive types with the same primitive type name but different original types be unioned?

Reporter: Cheng Lian / @liancheng
Assignee: Cheng Lian / @liancheng

Related issues:

• Release Parquet-mr 1.9.0 (blocks)
• PrimitiveType.union accepts fixed_len_byte_array fields with different lengths when strict mode is on (is related to)
• GroupType.union() doesn't merge the original type (is related to)

PRs and other links:

• PARQUET-385 PARQUET-379: Fixes strict schema merging #315

_{Note: This issue was originally created as PARQUET-379. Please see the migration documentation for further details.}

[asfimport]

Cheng Lian / @liancheng:
While trying to fix this issue, I got a problem regarding to the strict argument of PrimitiveType.union, and correspondingly, MessageType.union. Seems that throughout the whole parquet-mr code base (including tests), we always call these methods with strict being true, which means schema primitive types should match.

Maybe I missed something here, but I don't see a sound use case of non-strict schema merging. Especially, the field types of t1.union(t2, false) is completely determined by t1, rather than the "wider" ones:

message t1 { required int32 f; }
message t2 { required int64 f; }

t1.union(t2, false) =>
  message t3 { required int32 f; }

Basically we can't use such a schema to read actual Parquet files even if we add some sort of automatic "type widening" logic inside Parquet readers since the merged one above loses precision.

So my questions are:

1. Is there a practical scenario where non-strict schema merging makes sense?
2. If not, should we deprecate it? (We can't remove it since MessageType.union(MessageType, boolean) is part of the public API.)

[asfimport]

Ryan Blue / @rdblue:
I think this is part of a larger issue of handling schema evolution. The main use case I know of for union is merging file schemas into a metadata summary file. Those are no longer really needed because each schema is resolved against the requested schema individually on the reader, which eliminates the bottle-neck that the metadata file was intended to avoid. And as you note, union doesn't really create a union as one might expect: a schema that can be used to read both of the input schemas.

[asfimport]

Cheng Lian / @liancheng:
So deprecating non-strict schema merging seems to be reasonable? Namely, deprecate MessageType.union(MessageType toMerge, boolean strict), and always set strict to true when we call this method internally.

[asfimport]

Julien Le Dem / @julienledem:
[~lian cheng] Did you have a patch for this.

[asfimport]

Cheng Lian / @liancheng:
Just opened one for this: #315

[asfimport]

Julien Le Dem / @julienledem:
Issue resolved by pull request 315
#315