ARROW-2384: [Rust] Additional test & Trait standardization by max-sixty · Pull Request #1818 · apache/arrow

max-sixty · 2018-04-01T07:04:15Z

A test for the string representation of a Schema.

& some standardization on Traits rather than their method

...but mainly getting my feet wet with rust & arrow

andygrove · 2018-04-01T15:56:28Z

LGTM

wesm · 2018-04-01T21:20:49Z

Can you create a JIRA? @cpcloud @xhochy @pitrou could I ask you to help with maintaining the Rust patches while I am away for the project the rest of this month?

pitrou · 2018-04-01T21:42:15Z

I've never used Rust but I can provide non-technical guidance.

sadikovi

Thanks a lot for making the change! I just left some optional minor comments that could improve readability.

sadikovi · 2018-04-02T01:49:03Z

rust/src/datatypes.rs


 impl Field {
-
+    


Can you remove this whitespace?

Shall we rustfmt the whole thing?

I don’t know, I am not very familiar with how Rust project is set up for Arrow:). It might be easier to fix manually, if you want to.

@andygrove OK with you to run rustfmt on all the files?

👍 to rustfmt-ing everything. Makes it much easier for new contributers.

sadikovi · 2018-04-02T02:02:45Z

rust/src/datatypes.rs

-        let s : Vec<String> = self.columns.iter()
+impl fmt::Display for Schema {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let s: Vec<String> = self.columns


Very minor, but you could consider using write!, like you added, e.g.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { for (i, col) in self.columns.iter().enumerate() { write!(f, "{}", col)?; if i < self.columns.len() - 1 { write!(f, ", ")?; } } Ok(()) }

OK great, thanks for the pointer, I'll make that change.

Why do we need the ? at the end of the lines there?

? is a special shortcut token that is basically unrolled into something like this:

match write!(f, "...") { Ok(...) => // do something, Err(...) => // return err immediately, }

You would see it very often in the code, it is quite handy for unwrap Result with very minimum amount of code.

sadikovi · 2018-04-02T02:03:49Z

rust/src/datatypes.rs

            .map(|c| c.to_string())
            .collect();
-        s.join(",")
+        write!(f, "{}", s.join(","))


Would you like to keep a whitespace after comma? IMHO, it makes it easier to read the schema.

I agree, but is there a standard? Ideally we'd test against that

Good question! I actually do not know, and asked similar question down in the comments thread.

sadikovi · 2018-04-02T02:04:19Z

rust/src/datatypes.rs

-                Field::new("street", DataType::Utf8, false),
-                Field::new("zip", DataType::UInt16, false),
-            ]), false),
+                    Field::new("street", DataType::Utf8, false),


I think the shift might have been accidental here:)

sadikovi · 2018-04-02T02:06:48Z

rust/src/datatypes.rs

 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
+use std::fmt;


nit: I think, based on other source files, we keep a new line after the license header.

sadikovi · 2018-04-02T02:32:01Z

@wesm @andygrove Is there a standard way of printing schema like in Parquet (with message type)?

pitrou · 2018-04-02T11:20:16Z

Side note: if you choose to use an automatic formatter on Rust code, you should probably add a lint step to the CI setup.

pitrou · 2018-04-02T11:24:59Z

Is there a standard way of printing schema like in Parquet (with message type)?

I don't think a "standard" is mandated, but see arrow::PrettyPrint and also pretty_print-test.cc in the C++ codebase. You may want to reproduce something similar.

For the specific question in the comments, adding a space after commas is good typographical practice.

andygrove · 2018-04-02T13:29:12Z

I created a JIRA for setting up rustfmt: https://issues.apache.org/jira/browse/ARROW-2378

…

On Mon, Apr 2, 2018 at 5:25 AM, Antoine Pitrou ***@***.***> wrote: Is there a standard way of printing schema like in Parquet (with message type)? I don't think a "standard" is mandated, but see arrow::PrettyPrint and also pretty_print-test.cc in the C++ codebase. You may want to reproduce something similar. For the specific question in the comments, adding a space after commas is good typographical practice. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1818 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA5AxNbSOYa5lfa19SyOOUp1LkzyeOBbks5tkgqQgaJpZM4TCvPW> .

max-sixty · 2018-04-02T19:05:23Z

@andygrove should I rustfmt for this PR or a separate one? There's not that much here - was mainly a way of getting my feet wet. But fine to split up if you prefer

wesm · 2018-04-02T19:28:15Z

@pitrou I mean more helping with PR workflow. This PR has an unclear title and does not reference any JIRA. At some point, someone will need to merge the patch and make sure the JIRA metadata is correct (fix version, assignee, component)

xhochy · 2018-04-03T04:46:10Z

@wesm Yes, I can. Probably not going to have time to learn Rust but taking care of titles, consensus in PR comments & co is manageable.

As discussed in #1818 Probably worth merging fairly soon to minimize conflicts. I'll iterate quickly on any feedback. Author: Maximilian Roos <m@maxroos.com> Closes #1825 from maxim-lian/rustfmt and squashes the following commits: bfeeb54 <Maximilian Roos> rustfmt-preview fe7c228 <Maximilian Roos> move cargo fmt to rust script e224515 <Maximilian Roos> hanging space 2c6375b <Maximilian Roos> travis lint 8a6f223 <Maximilian Roos> cargo fmt

xhochy · 2018-04-03T04:48:56Z

Needs to be rebased on master after the rustfmt and other patches were merged.

max-sixty · 2018-04-03T05:01:05Z

rust/src/datatypes.rs

+            .iter()
+            .map(|c| c.to_string())
+            .collect::<Vec<String>>()
+            .join(", "))


@sadikovi what do you think of this? I will revert to your suggestion if you prefer. This feels nicer from a functional POV (but I'm only starting rust)

Looks good, I suggest you keep your version.

max-sixty · 2018-04-03T05:24:17Z

rust/src/buffer.rs

    }
 }

-impl<T> Drop for Buffer<T> {


master doesn't compile because this is also at https://github.com/apache/arrow/pull/1818/files/5449cc074163ec160e366ad5be84422cfdea7ccb#diff-efb40dbeb7a3acab561ac659c2cfee74R67

@maxim-lian Would you be able to fix this in a PR?

Yes - this the fix!

max-sixty · 2018-04-04T02:17:21Z

Gentle ping @sadikovi

sadikovi

Looks good. Great to see formatted code! Thanks for making all the changes!

sadikovi · 2018-04-04T03:32:22Z

rust/src/datatypes.rs

+            .iter()
+            .map(|c| c.to_string())
+            .collect::<Vec<String>>()
+            .join(", "))


Looks good, I suggest you keep your version.

max-sixty · 2018-04-04T03:49:23Z

Thanks @sadikovi !

@xhochy - lmk if anything else is needed before merging

xhochy · 2018-04-04T12:32:14Z

@maxim-lian I think there are some rebase artifacts in here due to merges to master. Can you rebase again?

max-sixty · 2018-04-04T17:50:29Z

@xhochy updated!

max-sixty added 2 commits April 1, 2018 01:47

test and impl

945caa7

display for Field

2e3b4c0

sadikovi approved these changes Apr 2, 2018

View reviewed changes

max-sixty added 3 commits April 2, 2018 19:51

cargo fmt

8a6f223

travis lint

2c6375b

hanging space

e224515

max-sixty mentioned this pull request Apr 3, 2018

ARROW-2378: [Rust] Rustfmt #1825

Closed

max-sixty added 4 commits April 2, 2018 20:53

move cargo fmt to rust script

fe7c228

rustfmt-preview

bfeeb54

cargo fmt

260ba24

Merge branch 'rustfmt' into rust-tweak

58273d8

max-sixty added 3 commits April 3, 2018 00:55

changes from @sadikovi

1a6d6d4

merge

5449cc0

fmt

730050b

max-sixty commented Apr 3, 2018

View reviewed changes

max-sixty changed the title ~~Rust test~~ ARROW-TBD: [Rust] Additional test & Trait standardization Apr 3, 2018

max-sixty commented Apr 3, 2018

View reviewed changes

max-sixty changed the title ~~ARROW-TBD: [Rust] Additional test & Trait standardization~~ ARROW-2384: [Rust] Additional test & Trait standardization Apr 3, 2018

sadikovi approved these changes Apr 4, 2018

View reviewed changes

max-sixty added 4 commits April 4, 2018 12:43

Merge branch 'master' into rust-tweak

c29b667

cargofmt

8879b65

merge

6c33977

no need for display for datatype yet

6b3fd47

xhochy closed this in 486d592 Apr 5, 2018

Conversation

max-sixty commented Apr 1, 2018

Uh oh!

andygrove commented Apr 1, 2018

Uh oh!

wesm commented Apr 1, 2018

Uh oh!

pitrou commented Apr 1, 2018

Uh oh!

sadikovi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadikovi Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadikovi commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Apr 2, 2018

Uh oh!

pitrou commented Apr 2, 2018

Uh oh!

andygrove commented Apr 2, 2018 via email

Uh oh!

max-sixty commented Apr 2, 2018

Uh oh!

wesm commented Apr 2, 2018

Uh oh!

xhochy commented Apr 3, 2018

Uh oh!

xhochy commented Apr 3, 2018

Uh oh!

max-sixty Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-sixty Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-sixty commented Apr 4, 2018

Uh oh!

sadikovi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-sixty commented Apr 4, 2018

Uh oh!

sadikovi Apr 2, 2018 •

edited

Loading

sadikovi commented Apr 2, 2018 •

edited

Loading

max-sixty Apr 3, 2018 •

edited

Loading

max-sixty Apr 3, 2018 •

edited

Loading