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

SchemaUtils Nested Map Fix #499

Merged

Conversation

jescalada
Copy link
Contributor

This PR fixes the issue where files generated by 3rd-party tools (with nonstandard schemas) fail to read correctly.

Let me know if the new test is comprehensive enough. I tried to add another test by generating a nested schema recursively, but this would require modifying DoRoundtripTest by changing the keys/values from 2D to 3D arrays.

This was the schema generation function I thought of, but I don't know if this is overkill for our purposes.

        private static GroupNode CreateLogicalTypeMapSchema(bool nested)
        {
            using var stringType = LogicalType.String();
            using var mapType = LogicalType.Map();

            using var keyNode = new PrimitiveNode("key", Repetition.Required, stringType, PhysicalType.ByteArray);
            using var primitiveValueNode = new PrimitiveNode("value", Repetition.Optional, stringType, PhysicalType.ByteArray);
            using var valueNode = nested
                ? new GroupNode("value", Repetition.Optional, new Node[] {keyNode, primitiveValueNode}, mapType)
                : (Node)primitiveValueNode;
            using var keyValueNode = new GroupNode(
                "key_value", Repetition.Repeated, new Node[] {keyNode, valueNode}, mapType);
            using var colNode = new GroupNode(
                "col1", Repetition.Required, new Node[] {keyValueNode}, mapType);

            return new GroupNode(
                "schema", Repetition.Required, new Node[] {colNode});
        }

Copy link
Contributor

@adamreeve adamreeve left a comment

Choose a reason for hiding this comment

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

This looks good thanks Juan, I just have some comments about the test

csharp.test/TestMaps.cs Outdated Show resolved Hide resolved
csharp.test/TestMaps.cs Outdated Show resolved Hide resolved
csharp.test/TestMaps.cs Outdated Show resolved Hide resolved
@jescalada jescalada requested a review from adamreeve January 24, 2025 04:18
csharp.test/TestMaps.cs Outdated Show resolved Hide resolved
@adamreeve adamreeve merged commit a6630c0 into G-Research:master Jan 24, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants