Unexpected schema merging nullability behavior

[brancz]

Describe the bug

When merging two schemas using Schema::try_merge where one side doesn't have a column but the other does, then it keep the nullability setting of the preexisting column. However, this semantically doesn't make sense, the merged version of that would be the field with nullable being true, since that is the implicit property of the schema that doesn't have the field.

Consequently what this means is that it's impossible to merge schemas and therefore record batches where one side has a field that is nullable false, and the other doesn't have it at all.

To Reproduce

    #[test]
    fn test_schema_merge_nullability() {
        let merged = Schema::try_merge(vec![
            Schema::new(vec![
                Field::new("first_name", DataType::Utf8, false),
            ]),
            Schema::new(vec![
                Field::new("last_name", DataType::Utf8, false),
            ]),
        ])
        .unwrap();

        assert_eq!(
            merged,
            Schema::new(
                vec![
                    Field::new("first_name", DataType::Utf8, true),
                    Field::new("last_name", DataType::Utf8, true),
                ],
            )
        );
    }

Expected behavior

The above test passes.

Additional context

This of course assumes that the whole intention of schema merging is to merge record batches of merged schemas, so if that is a wrong assumption this can be solved other ways for us by creating our own schema merge functionality, however, I see no useful reason to merge schemas and not also merge record batches. And if that's the intention then I think this is a legitimate bug.

@alamb @vegarsti

[vegarsti]

take

[vegarsti]

Agree that this seems like a legitimate bug. The nullability going from false to true when the field exists on both schemas works as expected, test_schema_merge covers that

arrow-rs/arrow-schema/src/schema.rs

Line 1330 in 2bc269c

fn test_schema_merge() {

The reason this is happening is that in try_merge we simply iterate over all the schemas and merge each field, so there's no difference between building up the first schema (where the nullability should be preserved) and merging other schemas into that schema.

arrow-rs/arrow-schema/src/schema.rs

Lines 295 to 319 in 2bc269c

	pub fn try_merge(schemas: impl IntoIterator<Item = Self>) -> Result<Self, ArrowError> {
	let mut out_meta = HashMap::new();
	let mut out_fields = SchemaBuilder::new();
	for schema in schemas {
	let Schema { metadata, fields } = schema;
	// merge metadata
	for (key, value) in metadata.into_iter() {
	if let Some(old_val) = out_meta.get(&key) {
	if old_val != &value {
	return Err(ArrowError::SchemaError(format!(
	"Fail to merge schema due to conflicting metadata. \
	Key '{key}' has different values '{old_val}' and '{value}'"
	)));
	}
	}
	out_meta.insert(key, value);
	}
	// merge fields
	fields.iter().try_for_each(|x| out_fields.try_merge(x))?
	}
	Ok(out_fields.finish().with_metadata(out_meta))
	}

Might there be use cases where this existing behavior is a feature and not a bug?

[brancz]

I can't imagine a use case where merging two schemas isn't to also then merge/concat the record batches of the unified schema. But perhaps I'm only seeing my own use case, so if someone can come up with one, I'm all ears.

[vegarsti]

Pushed a PR here: #8851

[alamb]

When merging two schemas using Schema::try_merge where one side doesn't have a column but the other does, then it keep the nullability setting of the preexisting column. However, this semantically doesn't make sense, the merged version of that would be the field with nullable being true, since that is the implicit property of the schema that doesn't have the field.

I agree this doesn't make sense. I will look at the PR now

[alamb]

So the topic (and pain) of schema merging comes up a bunch in DataFusion (and perhaps elsewhere)

For example, here is the code in DataFusion that handles schema merging for Parquet

https://github.com/apache/datafusion/blob/6ab4d216b768c9327982e59376a62a29c69ca436/datafusion/datasource-parquet/src/file_format.rs#L406-L421

We also have a similar challenge when comparing schemas (in some cases a field that is less nullable than another should be compatibility).

I am not quite sure what merging logic belongs in what crate (e.g. I don't have a sense for if there is a broadly agreed upon definition of what schema merging means for schema outsides the context of the schema evolution context of DataFusion)

Thus what I suggest is:

1. We start by moving schema merging logic into DataFusion, and iterate there until we get it right
2. the consider if the logic belongs upstream in arrow-rs or not where we can commit to an API

Given all the various potential options, the API I would suggest is some sort of Merger structure. Something like

let mut schema_merger = SchemaMerger::new()
  .with_preserve_nulls(true); // set various options builder style;

// try to merge the schemas
schema_merger.try_merge(schemas)?;

// get the built schema
let merged_schema = schema_merger.build()?

[brancz]

I like that idea of creating a new API in datafusion first. Only suggestion I would have is to not even have the option to preserve nullability at first until there is a clear use case (I still struggle to come up with one). Having the schema merger like that though makes sense to me so we can extend it without breaking APIs.

[vegarsti]

Yeah, this sounds like a good idea! And the API seems nice.

[alamb]

Only suggestion I would have is to not even have the option to preserve nullability at first until there is a clear use case (I still struggle to come up with one).

Make sense to me -- the only potential reason I can see is if we need to preserve the existing (strange) semantics for some reason. Defaulting to the more reasonable nullability behavior makes sense to me

[vegarsti]

To clarify, are you @alamb saying you're in favour of changing the existing semantics with something like the linked PR, in parallel to building this API in DataFusion?

[alamb]

To clarify, are you @alamb saying you're in favour of changing the existing semantics with something like the linked PR, in parallel to building this API in DataFusion?

I suggest starting with an API in DataFusion where we can work out whatever the most useful semantics are. Then when we port it back upstream to arrow we can figure out what the best defaults, etc

[vegarsti]

To clarify, are you @alamb saying you're in favour of changing the existing semantics with something like the linked PR, in parallel to building this API in DataFusion?

I suggest starting with an API in DataFusion where we can work out whatever the most useful semantics are. Then when we port it back upstream to arrow we can figure out what the best defaults, etc

Yep, sounds good! Thanks