Skip to content

Conversation

@bkietz
Copy link
Member

@bkietz bkietz commented Apr 30, 2021

Cleanup PR after ARROW-11929 which added untested support for ExecuteScalarExpression(Table).

@github-actions
Copy link

@nealrichardson
Copy link
Member

I started deleting some R code in #10191 by using InMemoryDataset; can we discuss tradeoffs of the two approaches?

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

LGTM, though there's one MSVC error to be fixed.

Regardless of whether we go a different direction, we'll probably want this since the current code path simply bails unexpectedly/crashes.

@bkietz
Copy link
Member Author

bkietz commented Apr 30, 2021

The InMemoryDataset approach is more aligned with dplyr, so if that's straightforward and sufficient then I'd say we probably don't want this (and I'll delete the Datum::TABLE) path. If there's interest in replacing simpler vector expressions with calls to ExecuteScalarExpression, then this might be still be useful for things like Array$create(c(1,2,3)) + Array$create(c(1,4,5))

@lidavidm
Copy link
Member

The AppVeyor build is still failing due to a syntax error, oddly.

@bkietz bkietz changed the title ARROW-12614: [C++][Compute] Support Tables in ExecuteScalarExpression ARROW-12614: [C++][Compute] Remove support for Tables in ExecuteScalarExpression May 3, 2021
@lidavidm lidavidm closed this in 9af99c5 May 3, 2021
@bkietz bkietz deleted the 12614-Add-support-for-Tables-to branch May 3, 2021 18:08
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.

3 participants