Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Enable backward compatible map reads #418

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

parsnips
Copy link
Contributor

@parsnips parsnips commented Dec 1, 2022

Implements a deprecated.MapGroup that applies to maps written by older parquet clients.

For example, AWS Kinesis Firehose's ParquetSerDe will write maps with the following schema fragment:

	optional group new {
		repeated group map {
			required binary key (STRING);
			optional group value {
				optional binary b (STRING);
				optional binary n (STRING);
			}
		}
	}
type Example struct {
    New  deprecated.MapGroup[string]string `parquet:"new,optional"`
}

Resolves #415

@achille-roussel
Copy link

Thanks for submitting this change @parsnips !

I would like to make a suggestion, instead of using a new struct tag, would it be possible to handle this special case with a type?

We have a deprecated package intended for this purpose, how about adding a type such as:

package deprecated

type Map[K comparable, V any] map[K]V

What do you think?

@parsnips
Copy link
Contributor Author

parsnips commented Dec 2, 2022

An excellent suggestion, commit incoming :)


// MapGroup is an implementation of the deprecated `repeated group map`
// syntax that older parquet writers emit.
type MapGroup[K comparable, V any] map[K]V

Choose a reason for hiding this comment

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

What made you choose MapGroup rather than just Map? Is this term used in the parquet specification or in other client libraries?

Copy link
Contributor Author

@parsnips parsnips Dec 5, 2022

Choose a reason for hiding this comment

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

There's a constant in the package already named Map

Open to a good suggestion, I thought about DeprecatedMap and was too long, and then named so because of the syntax of the parquet being repeated map group ...so I just called this the map group :)

Choose a reason for hiding this comment

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

Ah good call :) Thanks for pointing it out 👍

- Implement an `IsMap` and hide the reflected interfaces
- renamed `OldMap` to `deprecatedMap` to make less likely to be exported
Copy link

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

This is looking great!

One thing to take into account is merging this change will move the minimum Go version required by the package to 1.18 since we are creating a dependency on a generics type.

We gave #428 opened to get this work done.

I'll leave it up to you to decide whether to make this code compatible with Go 1.17, or we can put the change on a short hold until we are ready to drop backwards compatibility (I would expect we get to this by end of year).

Thanks again for your contribution!

@parsnips
Copy link
Contributor Author

parsnips commented Dec 5, 2022

@achille-roussel I think we're ok with waiting for a go 1.18 only support. I was planning on supporting a fork for the period of time until this change gets a release anyway... .and I'm not looking back ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable backwards compatible reads on maps
2 participants