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

Interest in support for automatic column mapping in RowOriented reader? #90

Closed
bmacphee opened this issue Jan 20, 2020 · 4 comments
Closed
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@bmacphee
Copy link
Contributor

bmacphee commented Jan 20, 2020

Based on this issue (#72), it seems like the row-oriented reader is not really intended to be used as a core part of the library (more of an example).

Is there any interest in a PR to add some minor enhancements to the row oriented API? Specifically, I needed to pass some parquet data to and from a component that doesn't guarantee column order is preserved. When I read it back, I needed to map the columns to my TTuple type by name, rather than by position. I know that can't work for all possible usages of parquet but it was convenient.

@GPSnoopy
Copy link
Contributor

GPSnoopy commented Jan 20, 2020

Hi @bmacphee,

You are correct. The row oriented API makes too many trade-offs between ease of use, flexibility and performance. It's there to make it easy for new starters to read a Parquet file, but not heavily encouraged.

Nonethless, the feature you might be looking for is the addition of a C# field attribute to explictly state that a tuple field maps to a particular column name. For example:

struct MyTuple
{
    [MapToColumnName("Index")]
    int Item1;

    [MapToColumnName("Value")]
    float Item2;
}

Orthogonally one can imagine a ColumnOrder(int) attribute to reflect the current implicit column ordering and make it explicit. Things get messy when you start mixing things together (i.e. how much flexibility do you allow?).

Perhaps as a first start, if MapToColumnName is used, then all columns need to be marked with this attribute (and throw an exception if not). I'd suggest looking at GetFieldsAndProperties() in RowOriented/ParquetFile.cs and working up from there. Afterall, the introduction of such a field attribute would only change the ordering of the resulting array.

Edit: I cannot decide whether it should be called ColumnName or MapToColumnName. The former describe an explicit column name, while the latter hints at a different ordering when reading.

@GPSnoopy GPSnoopy added enhancement New feature or request question Further information is requested labels Jan 20, 2020
@GPSnoopy
Copy link
Contributor

I've also noticed that GetFieldsAndProperties() has little ordering guarantees if you start mixing fields and properties, as they are listed by type (fields first, properties second).

@bmacphee
Copy link
Contributor Author

bmacphee commented Jan 20, 2020

I did consider using attributes as a way to expose this to potential users without having to have a mode toggle argument or something. My initial proof of concept just did it by relying on what's returned from GetFieldsAndProperties being exact matches (case sensitive) for the parquet file, which was totally fine for my purposes.

I agree it would make sense to enforce the all-or-nothing approach with the column names, even if the attributes just end up echoing the field names. It's still convenient enough as you only need to write the data class once.

I don't have a diff to show right this second but what I did was add this to the ParquetRowReader construction:

	// TODO: validate types?
	// TODO: conditions for handling bare tuples for TTuple type?
	Dictionary<string, int> fileColumnIndexes = new Dictionary<string, int>();
	for (int i = 0; i < _parquetFileReader.FileMetaData.Schema.NumColumns; ++i)
	{
		fileColumnIndexes[_parquetFileReader.FileMetaData.Schema.Column(i).Name] = i;
	}
	for (int i = 0; i < fields.Length; ++i)
	{
		_objectToFileColumnMapping[i] = fileColumnIndexes[fields[i].name];
	}

and then ReadColumn just looks up the "correct" column when asked by changing the index.

I appreciate the input.

@GPSnoopy
Copy link
Contributor

Merging into master as part of PR #108.

@GPSnoopy GPSnoopy self-assigned this Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants