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

ARROW-13888: [R] Rephrase docs for schema()'s ellipses argument and rephrase error message #11645

Closed

Conversation

thisisnic
Copy link
Member

No description provided.

@github-actions
Copy link

github-actions bot commented Nov 8, 2021

@github-actions
Copy link

github-actions bot commented Nov 8, 2021

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Copy link
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

🙏 thanks for this! Looking good, a few questions

r/R/schema.R Outdated

schema_check <- function(items) {
all(map_lgl(.list, ~ inherits(.x, "Field"))) || FALSE
}
Copy link
Member

Choose a reason for hiding this comment

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

Is schema_check used elsewhere? I don't see it but I might be missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, intended to use it and then didn't, will update

Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful as a helper especially if we accept a list as input (or for testing). But up to you if you think it's better as a helper or inline

Copy link
Member Author

Choose a reason for hiding this comment

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

Had to refactor now so dropped it as it no longer works

r/R/schema.R Outdated
@@ -157,6 +165,9 @@ Schema$create <- function(...) {
.list <- list2(...)
if (all(map_lgl(.list, ~ inherits(., "Field")))) {
schema_(.list)
} else if (is.null(names(.list))) {
# If not supplied as fields, only other alternative is e.g.`a = int16()`
abort("Schema definitions must be supplied as field/data type or field name/data type pairs")
} else {
schema_(.fields(.list))
}
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible (and if so wise?) to check if length(.list) == 1 && schema_check(.list[[1]]) then schema_(.fields(.list[[1]]))?

One could always call do.call(schema, list), but it might be nice to also accept a single list containing the specifications too?

Copy link
Member Author

@thisisnic thisisnic Nov 9, 2021

Choose a reason for hiding this comment

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

Sure, though would we want to also expand it to check if length(.list) == 1 && all(map_lgl(.list, ~ inherits(.x, "DataType"))) and allow that too?

Copy link
Member Author

@thisisnic thisisnic Nov 9, 2021

Choose a reason for hiding this comment

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

(i.e. so the user can supply a list of name/value pairs and it's not important which of the 2 methods they use to specify the name - fields or just field names)

Copy link
Member

Choose a reason for hiding this comment

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

OH, yes absolutely (if it's not too much scope creep here!) that would be quite nice

)

expect_equal(name_list_schema, schema(b = double(), c = string(), d = int8()))
expect_equal(field_list_schema, schema(b = double(), c = bool(), d = string()))
Copy link
Member

Choose a reason for hiding this comment

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

😍

Copy link
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

Oops, thought I sent this yesterday, but apparently I clicked the wrong button!

r/R/schema.R Show resolved Hide resolved
Co-authored-by: Jonathan Keane <jkeane@gmail.com>
@thisisnic thisisnic closed this in a374604 Nov 11, 2021
@ursabot
Copy link

ursabot commented Nov 11, 2021

Benchmark runs are scheduled for baseline = 225d954 and contender = a374604. a374604 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

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

Successfully merging this pull request may close these issues.

None yet

3 participants