Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVRO-3155: [docs] Remove 'default' from map and array examples (and add to enum). #3046

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

HuwCampbell
Copy link
Contributor

@HuwCampbell HuwCampbell commented Jul 25, 2024

AVRO-3155

What is the purpose of the change

Default values have no special purpose for maps and arrays, and are only
valid for record fields and enums.

Having it in the examples is confusing, as people might have expectations
around it behaving in a certain way.

In the Java and Rust implementation, a default value listed here
will be packed into the custom props and attributes payloads, and
do not need to be of the same type; i.e., default: "no default" would
be valid.

Other implementations just ignore it and won't round trip it, for example:

MapType.prototype.toJSON = function () {
  return {type: 'map', values: this._values};
};

Verifying this change

I wrote a small test in rust to validate my understanding; unfortunately
using an assertion on Map would be confusing as the instance
for Eq for MapSchema ignores the attributes field.

#[test]
fn avro_test_map_default() -> TestResult {
    // string uuid, represents as native logical type.
    let schema = json!(
    {
        "type": "map",
        "values": "long",
        "default": "4"
    });
    let parse_result = Schema::parse(&schema)?;
    assert_eq!(parse_result, Schema::Uuid);

    Ok(())
}

Resulting in:

assertion `left == right` failed
  left: Map(MapSchema { types: Long, attributes: {"default": String("4")} })

For Java I just read the code.

Default values are not valid for maps and arrays, and are only
valid for record fields (and enums, though that's a different story).

Having it in the examples is confusing, as people might
have expectations around it behaving in a certain way.

In the Java and Rust implementation, a default value listed here
will be packed into the custom props and attributes payloads, and
do not need to be of the same type.
Copy link
Contributor

@xxchan xxchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originates from
#662 https://issues.apache.org/jira/browse/AVRO-2574 It doesn't mention any reason though.

cc @Fokko

Copy link
Contributor

@xxchan xxchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me

@xxchan
Copy link
Contributor

xxchan commented Jul 27, 2024

BTW, I don't think Rust implementation is stable enough as a reference. Can you link to related Java code?

@HuwCampbell
Copy link
Contributor Author

HuwCampbell commented Jul 27, 2024

This is in Schema.java.

@Override
@Deprecated
void toJson(Set<String> knownNames, String currentNamespace, JsonGenerator gen) throws IOException {
  gen.writeStartObject();
  gen.writeStringField("type", "map");
  gen.writeFieldName("values");
  valueType.toJson(knownNames, currentNamespace, gen);
  writeProps(gen);
  gen.writeEndObject();
}

Props are written, but there's no default value to be seen.

On the parsing side, the code for the parser code also just reads the value field here.

@HuwCampbell
Copy link
Contributor Author

Is there anything else required to get this merged? Like it's not critical, but it's weird to leave it hanging.

@martin-g
Copy link
Member

martin-g commented Aug 5, 2024

@Fokko Do you have any comments about these defaults ?
They have been introduced by you with #662

@KalleOlaviNiemitalo
Copy link
Contributor

This pull request seems to fix [AVRO-3155] Schema specification inconsistently describes default value for types.

@martin-g martin-g changed the title [docs] Remove 'default' from map and array examples (and add to enum). AVRO-3155: [docs] Remove 'default' from map and array examples (and add to enum). Aug 5, 2024
@opwvhk opwvhk merged commit ec698a5 into apache:main Aug 29, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants