Refactor Row #89

sunchao · 2018-04-15T19:41:00Z

This breaks the Row struct into two parts: a RowField
which represent a single field in a row, and a top-level Row
struct which represents the value for a message or struct type.
Only the latter is exposed as public API.

coveralls · 2018-04-16T02:36:52Z

Coverage remained the same at 94.898% when pulling ef04ba8 on refactor-row into 9681558 on master.

sadikovi · 2018-04-16T07:42:43Z

Thanks for making the change!

First of all, sorry for the long and funky tests in record module that you had to change.
And for a conflicting files in this PR.

Looks good, I have a couple questions though.

Now Row represents the top level struct and RowField::Group represents the some internal struct. But they both would need getXXX methods then. Will it result in code duplication? By the way, are getXXX methods ones that are part of RowAccessor in another PR?

With the current approach we can map Row to some other structure, e.g. JSON struct. Is it still possible with this update?

sadikovi · 2018-04-16T07:43:56Z

Another thing - should we rename Row to something else? I just think it is a bit weird name for a record:).

sunchao · 2018-04-16T08:01:30Z

Thanks for taking a look @sadikovi .

Now Row represents the top level struct and RowField::Group represents the some internal struct. But they both would need getXXX methods then. Will it result in code duplication? By the way, are getXXX methods ones that are part of RowAccessor in another PR?

Only Row needs to implement the getXXX methods, and we don't need to change anything on RowField::Group (calling getStruct on a Row will yield another Row). And yes, these getXXX methods are the same as in the other PR.

With the current approach we can map Row to some other structure, e.g. JSON struct. Is it still possible with this update?

The mapping can still happen at the RowField level, which not much has changed. We just added a public Row API with a few getters.

Another thing - should we rename Row to something else? I just think it is a bit weird name for a record:).

I'm debating on Row or Record, and BTW, Spark also uses Row for similar purpose. Row also maps to row group. I don't like very much RowField though - maybe we can just call it Field or Column. I'm not quite sure yet...

sadikovi

Now it makes sense. It was a bit difficult for me to see it coming together in two separate PRs. As long as we can map Row enum and fields to something and get values, it is good.

Agree, we should just call it Field.

I left some minor comments. And sorry again for all those tests that you need to update.

sadikovi · 2018-04-16T10:26:36Z

src/record/api.rs

+  fields: Vec<(String, RowField)>
+}
+
+pub fn make_row(fields: Vec<(String, RowField)>) -> Row {


Can you add some doc to the function? Thanks.
Would it be better to keep it as macro, since it is used in different places? Would it be inlined? If so, then it is fine.

Sure. I'll mark it inline and document it.

sadikovi · 2018-04-16T10:28:05Z

src/record/mod.rs

 mod triplet;
+
+pub use self::api::Row;


Should we export RowField as well or will it work with just Row?

I think we should not expose RowField (or Field). For primitive types, the getters will return Rust native types, such as i32, bool etc. We also need to define and expose clean-defined interfaces for array and map. The former should also have getters like getBoolean(rowIndex: usize) to return the value at a particular row, and the latter should have something like getKeys(), getValues(), etc.

I'll take a further look into this. It seems rather verbose if we have to define getXXX for every data type on struct, array, etc. I wonder if there's a cleaner way to do it.

sadikovi · 2018-04-16T10:32:02Z

src/record/reader.rs

@@ -297,7 +298,24 @@ impl Reader {

  /// Reads current record as `Row` from the reader tree.
  /// Automatically advances all necessary readers.
+  /// This must be called on the root level reader (i.e., for Message type).
+  /// Otherwise, it will panic.
  fn read(&mut self) -> Row {


When do we call read? I am just a bit confused:)

The read is called on next() for both RowIter and ReaderIter.

sadikovi · 2018-04-16T20:32:52Z

src/record/api.rs

-  List(Vec<Row>), // List of elements
-  Map(Vec<(Row, Row)>) // List of key-value pairs
+  Group(Row), // Struct, child elements are tuples of field-value pairs
+  List(Vec<RowField>), // List of elements


I guess when it is list of structs, it will be List(Group(Row), Group(Row), ...). Is that right?

Yes it will be something like List(Vec(Group(Row), Group(Row),...)), and the array interface that I proposed above should have something like getStruct(rowIndex) which will return a Row.

Great, thanks.

Would it be beneficial to also make List and Map separate, similar to Row, so they can define their own set of methods to get length, elements by index, keys and values?

I'm thinking about to replace List<Vec<Row>> to something like List<Array> (we'll need better naming on this) where the Array implements the public APIs. Similar for Map<Vec<Row, Row>>. What do you think?

I remember you suggested this some time ago. Yes, sounds like a good idea. You could consider calling them Struct, Array, Map, Field, and we would return Struct for a top-level record, so it aligns with get_struct, get_array, and get_map methods.

sadikovi

Looks good. I guess, all TODOs will be fixed as part of field accessors work that is open right now.

Can you add docs for Map and List and rebase? Thanks!

sunchao · 2018-04-20T02:56:45Z

Yes all the TODOs will be fixed later. Will add doc for Map & List, and rebase. Thanks

This breaks the `Row` struct into two parts: a `RowField` which represent a single field in a row, and a top-level `Row` struct which represents the value for a message or struct type. Only the latter is exposed as public API.

sunchao · 2018-04-20T04:22:00Z

Merged. Thanks @sadikovi !

sadikovi · 2018-04-20T04:29:54Z

@sunchao No worries, thanks for the changes.

sunchao mentioned this pull request Apr 16, 2018

Add type-safe accessors to Row #85

Closed

sadikovi reviewed Apr 16, 2018

View reviewed changes

sunchao mentioned this pull request Apr 19, 2018

Add statistics for column chunk metadata and data page #94

Merged

sadikovi approved these changes Apr 20, 2018

View reviewed changes

sunchao added 5 commits April 19, 2018 20:03

Refactor Row

04d1066

This breaks the `Row` struct into two parts: a `RowField` which represent a single field in a row, and a top-level `Row` struct which represents the value for a message or struct type. Only the latter is exposed as public API.

change RowField to Field

90438ec

add List and Map

cb8dc01

tiny refactor

2a5f124

add doc

ef04ba8

sunchao force-pushed the refactor-row branch from 77e7345 to ef04ba8 Compare April 20, 2018 03:45

sunchao merged commit 1f3b8b9 into master Apr 20, 2018

sunchao deleted the refactor-row branch April 20, 2018 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Row #89

Refactor Row #89

sunchao commented Apr 15, 2018

coveralls commented Apr 16, 2018 •

edited

Loading

sadikovi commented Apr 16, 2018

sadikovi commented Apr 16, 2018

sunchao commented Apr 16, 2018

sadikovi left a comment

sadikovi Apr 16, 2018

sunchao Apr 16, 2018

sadikovi Apr 16, 2018

sunchao Apr 16, 2018

sadikovi Apr 16, 2018

sunchao Apr 16, 2018

sadikovi Apr 16, 2018 •

edited

Loading

sunchao Apr 16, 2018

sadikovi Apr 16, 2018

sunchao Apr 16, 2018

sadikovi Apr 16, 2018 •

edited

Loading

sadikovi left a comment

sunchao commented Apr 20, 2018

sunchao commented Apr 20, 2018

sadikovi commented Apr 20, 2018

Refactor Row #89

Refactor Row #89

Conversation

sunchao commented Apr 15, 2018

coveralls commented Apr 16, 2018 • edited Loading

sadikovi commented Apr 16, 2018

sadikovi commented Apr 16, 2018

sunchao commented Apr 16, 2018

sadikovi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadikovi Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadikovi Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

sadikovi left a comment

Choose a reason for hiding this comment

sunchao commented Apr 20, 2018

sunchao commented Apr 20, 2018

sadikovi commented Apr 20, 2018

coveralls commented Apr 16, 2018 •

edited

Loading

sadikovi Apr 16, 2018 •

edited

Loading

sadikovi Apr 16, 2018 •

edited

Loading