Add type-safe accessors for primitive types in Row #86

andygrove · 2018-04-13T23:13:37Z

No description provided.

coveralls · 2018-04-13T23:44:52Z

Coverage decreased (-0.009%) to 94.9% when pulling 76d4ab0 on andygrove:row_type_safe_accessors into 1f3b8b9 on sunchao:master.

sadikovi

Great! Thank you very much!

I left a comment and I have several general suggestions/questions:

Would it be useful to add method is_primitive that returns true if type is not Group/List/Map, and false otherwise?
Would it be possible to add tests to cover all value conversions, e.g. Byte, Short, Int, Long, etc.?
Would you mind changing line width to 90? We normally follow this style rule. Thanks.

sadikovi · 2018-04-13T23:48:09Z

src/record/api.rs

+    pub fn $METHOD(&self) -> Result<$TY, ParquetError> {
+        match *self {
+            Row::$VARIANT(v) => Ok(v),
+            _ => Err(ParquetError::General(format!("Cannot access {} as {}", self.get_type_name(), stringify!($VARIANT))))


I was thinking if it would be easier to just print self instead of self.get_type_name? One of the pros is that we need not maintain the mapping to string values, but I am happy either way.

If we print self, we would have a message like Cannot access 1.2 as Bool which makes debugging harder?

Fair enough, let's keep type names!

sadikovi · 2018-04-13T23:56:07Z

This should fix #85.

andygrove · 2018-04-14T00:20:48Z

I've added the remaining accessors as requested, and is_primitive.

andygrove · 2018-04-14T00:51:01Z

Looks like I need to add tests for the error condition for each type to keep code coverage up. I'll get that done either tonight or tomorrow.

sadikovi · 2018-04-14T00:53:04Z

Well, I think it is okay, but if you want to then it is even better! @sunchao what do you think?

andygrove · 2018-04-14T00:55:54Z

It was actually not much work after all.. I think it's great you have code coverage set up and I'll be learning from you and applying this to DataFusion soon

sunchao · 2018-04-14T01:29:05Z

Well, I think it is okay, but if you want to then it is even better! @sunchao what do you think?

Yes it would be great if we can cover those too but not a must.

sunchao · 2018-04-14T03:28:34Z

src/record/api.rs

+macro_rules! row_primitive_accessor {
+  ($METHOD:ident, $VARIANT:ident, $TY:ty) => {
+    pub fn $METHOD(&self) -> Result<$TY, ParquetError> {
+        match *self {


nit: we use 2-space indent.

sunchao · 2018-04-14T03:29:22Z

src/record/api.rs

+    pub fn $METHOD(&self) -> Result<$TY, ParquetError> {
+        match *self {
+            Row::$VARIANT(v) => Ok(v),
+            _ => Err(ParquetError::General(format!("Cannot access {} as {}", self.get_type_name(), stringify!($VARIANT))))


nit: maybe we can use general_err! here - a little less verbose
also we limit line width to 90 characters.

sunchao · 2018-04-14T03:32:04Z

src/record/api.rs

+// Macro to generate type-safe get_xxx methods e.g. get_bool, get_short
+macro_rules! row_primitive_accessor {
+  ($METHOD:ident, $VARIANT:ident, $TY:ty) => {
+    pub fn $METHOD(&self) -> Result<$TY, ParquetError> {


nit: you can import errors::Result and replace Result<$TY, ParquetError> with Result<$TY>

sunchao · 2018-04-14T03:35:34Z

src/record/api.rs

+  fn test_row_accessors_invalid_message() {
+    assert_eq!(ParquetError::General("Cannot access Float as Bool".to_string()),
+               Row::Float(1.2).get_bool().unwrap_err());
+    assert_eq!(ParquetError::General("Cannot access Float as Str".to_string()), Row::Float(1.2).get_string().unwrap_err());


nit: line too long.

sunchao · 2018-04-14T03:42:56Z

src/record/api.rs

+  row_primitive_accessor!(get_timestamp, Timestamp, u64);
+
+  /// Type-safe accessor for Str type
+  pub fn get_string(&self) -> Result<&String, ParquetError> {


One thing I'm not quite sure is whether it's better to just panic here, instead of returning Result. It seems pretty rare that the error case would happen, given that one always (I think?) will hold the schema of the row before calling these methods.

I think panic is pretty extreme. I think the user should decide if they want to panic or not. In non-test code they just need to add a ? at the end of the line, so it doesn't make much difference to code verbosity.

sunchao · 2018-04-20T04:24:38Z

Hi @andygrove , sorry for the delay! We just refactored the Row so that adding accessors to it might become easier. Could you take a look at the latest code and see if the changes can be applied to it (it should not be much difference).

andygrove · 2018-04-21T10:52:38Z

Cool. I am working on it this morning.

andygrove · 2018-04-21T11:42:40Z

@sunchao This is ready for review (as long as you are OK with the 0.009% decrease in code coverage!)

sadikovi

Thanks @andygrove. Appreciate you making changes. I left some minor comments, hope you could have a look. Thanks!

sadikovi · 2018-04-21T11:48:58Z

src/record/api.rs

@@ -41,6 +42,79 @@ pub struct Row {
  fields: Vec<(String, Field)>
 }

+impl Row {
+  /// Get then number of fields in this row


Should it be the?

sadikovi · 2018-04-21T11:49:32Z

src/record/api.rs

+}
+
+impl RowAccessor for Row {
+


Could you remove empty line?

sadikovi · 2018-04-21T11:49:50Z

src/record/api.rs

+  row_complex_accessor!(get_group, Group, Row);
+  row_complex_accessor!(get_list, ListInternal, List);
+  row_complex_accessor!(get_map, MapInternal, Map);
+


Could you remove empty line?

sadikovi · 2018-04-21T11:50:29Z

src/record/api.rs

@@ -71,6 +145,13 @@ pub struct List {
  elements: Vec<Field>
 }

+impl List {
+  /// Get then number of fields in this row


Should it be the?

sadikovi · 2018-04-21T11:50:53Z

src/record/api.rs

@@ -86,6 +167,13 @@ pub struct Map {
  entries: Vec<(Field, Field)>
 }

+impl Map {
+  /// Get then number of fields in this row


andygrove · 2018-04-21T11:56:58Z

@sadikovi I pushed the formatting changes.

Have you thought about adopting cargo fmt? Then you can just have the build fail if the formatting is off and this will no longer be a manual process.

sadikovi · 2018-04-21T12:03:06Z

Thanks for making changes. Looks good, we should merge it!

Yes, we discussed that and at the time, and we needed to apply only a subset of the rules, but I could not manage to configure it that way. So we decided to review manually and periodically do clean up with rustfmt.

sunchao

Thanks @andygrove ! PR looks good. Just one minor comment.

sunchao · 2018-04-21T20:17:29Z

src/record/api.rs

+}
+
+/// Trait for type-safe convenient access to fields within a Row
+trait RowAccessor {


This needs to be public and be exported in mod.rs, otherwise these methods will not be available.

andygrove · 2018-04-21T20:52:46Z

@sunchao Good catch! Fixed.

sunchao · 2018-04-21T21:13:43Z

Thanks @andygrove . Merged.

add type-safe accessors for primitive types in Row

abcf257

andygrove mentioned this pull request Apr 13, 2018

Add type-safe accessors to Row #85

Closed

sadikovi reviewed Apr 13, 2018

View reviewed changes

add accessors for all types (with tests) and also add is_primitive

198a1e3

add tests for errors on all type-safe accessors to keep code coverage up

d09907e

sunchao reviewed Apr 14, 2018

View reviewed changes

andygrove added 3 commits April 14, 2018 05:49

fix nits

acf66fa

implement RowAccessor trait

d1aeaa5

bug fix and more tests

910f118

sadikovi mentioned this pull request Apr 21, 2018

Releasing 0.2.0? #88

Closed

merge from master and re-implement type-safe accessors on Row

15533c2

sadikovi approved these changes Apr 21, 2018

View reviewed changes

andygrove added 2 commits April 21, 2018 05:54

fix typos, remove blank lines

e5c9b6e

remove more blank lines and remove old comment

89b04f9

sunchao reviewed Apr 21, 2018

View reviewed changes

make trait public

76d4ab0

sunchao merged commit 4f99ae1 into sunchao:master Apr 21, 2018

andygrove deleted the row_type_safe_accessors branch April 22, 2018 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type-safe accessors for primitive types in Row #86

Add type-safe accessors for primitive types in Row #86

andygrove commented Apr 13, 2018

coveralls commented Apr 13, 2018 •

edited

sadikovi left a comment

sadikovi Apr 13, 2018

andygrove Apr 14, 2018

sadikovi Apr 14, 2018

sadikovi commented Apr 13, 2018

andygrove commented Apr 14, 2018

andygrove commented Apr 14, 2018

sadikovi commented Apr 14, 2018

andygrove commented Apr 14, 2018

sunchao commented Apr 14, 2018

sunchao Apr 14, 2018

sunchao Apr 14, 2018

sunchao Apr 14, 2018

sunchao Apr 14, 2018

sunchao Apr 14, 2018

andygrove Apr 14, 2018

sunchao commented Apr 20, 2018

andygrove commented Apr 21, 2018

andygrove commented Apr 21, 2018

sadikovi left a comment

sadikovi Apr 21, 2018

sadikovi Apr 21, 2018

sadikovi Apr 21, 2018

sadikovi Apr 21, 2018

sadikovi Apr 21, 2018

andygrove commented Apr 21, 2018

sadikovi commented Apr 21, 2018

sunchao left a comment

sunchao Apr 21, 2018 •

edited

andygrove commented Apr 21, 2018

sunchao commented Apr 21, 2018

Add type-safe accessors for primitive types in Row #86

Add type-safe accessors for primitive types in Row #86

Conversation

andygrove commented Apr 13, 2018

coveralls commented Apr 13, 2018 • edited

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

sadikovi commented Apr 13, 2018

andygrove commented Apr 14, 2018

andygrove commented Apr 14, 2018

sadikovi commented Apr 14, 2018

andygrove commented Apr 14, 2018

sunchao commented Apr 14, 2018

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

sunchao commented Apr 20, 2018

andygrove commented Apr 21, 2018

andygrove commented Apr 21, 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

andygrove commented Apr 21, 2018

sadikovi commented Apr 21, 2018

sunchao left a comment

Choose a reason for hiding this comment

sunchao Apr 21, 2018 • edited

Choose a reason for hiding this comment

andygrove commented Apr 21, 2018

sunchao commented Apr 21, 2018

coveralls commented Apr 13, 2018 •

edited

sunchao Apr 21, 2018 •

edited