ARROW-8314: [Python] Add a Table.select method to select a subset of columns - #7272

Closed
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select
Closed

ARROW-8314: [Python] Add a Table.select method to select a subset of columns#7272
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select

Conversation

@jorisvandenbossche

Copy link
Copy Markdown
Member

This is a pure python implementation. It might be we want that on the C++ side (unless it already exists?), but having it available in Python is already useful IMO.

@github-actions

Copy link
Copy Markdown

@nealrichardson

nealrichardson commented May 26, 2020

Copy link
Copy Markdown
Member

Here's a C++ version, albeit from the R bindings: https://github.com/apache/arrow/blob/master/r/src/table.cpp#L128-L143

Since you're doing this in Python as well, maybe this should be moved to C++?

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche Do you need help on this?

@jorisvandenbossche
jorisvandenbosscheforce-pushed the ARROW-8314-table-select branch 2 times, most recently from 940fe20 to f3175e2CompareJune 22, 2020 14:03
@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I added a C++ version (didn't yet update R to use it)

Comment threadcpp/src/arrow/table.cc Outdated

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a small type problem on the C++ side

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a static_cast, or use int64_t or size_t instead.

@pitrou

Copy link
Copy Markdown
Member

It would also be nice to add a test on the C++ side, if that's not too time-consuming.

@nealrichardson

Copy link
Copy Markdown
Member

I added a C++ version (didn't yet update R to use it)

Are you intending to make that change in this PR too?

@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I updated this, and added a small C++ test.

Are you intending to make that change in this PR too?

Realistically speaking, not at the moment (I certainly want to learn how to set up a R dev environment, but just before the release with other priorities might not be the best time ;-))
So would either merge without, unless you can quickly push a change (I suppose it might be an easy change).

@nealrichardson

Copy link
Copy Markdown
Member

I added https://issues.apache.org/jira/browse/ARROW-9387 for using this in R. It might be trivial but in case it isn't I don't want to block this.

@jorisvandenbossche

jorisvandenbossche commented Jul 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou could you check the C++ test?

(fixing the linting issue)

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche I'll take a look when I'm done with 1.0.0-critical tasks. Hopefully before the end of the week :-)

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is returning Result anyway we might as well boundscheck the indices, thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a boundscheck

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Comment threadcpp/src/arrow/table_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ASSERT_OK(subset->ValidateFull())?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

+1

@wesmwesm closed this in 1413963Jul 14, 2020
@jorisvandenbossche
jorisvandenbossche deleted the ARROW-8314-table-select branch July 14, 2020 21:04
romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Sep 7, 2020
nealrichardson added a commit that referenced this pull request Sep 11, 2020
R follow up from #7272
The current `$select()` uses a more familiar (though more expensive) tidyselect interface:
``` r
library(arrow, warn.conflicts = FALSE)
tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
```
<sup>Created on 2020-09-07 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
Do we want both ? `$select()` is used e.g. by the `read_csv(col_select=)` argument:
```r
tab <- reader$Read()$select(!!enquo(col_select))
```
Closes#8125 from romainfrancois/ARROW-9387/Table_SelectColumns
Lead-authored-by: Romain Francois <romain@rstudio.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisvandenbossche@nealrichardson@pitrou@wesm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

ARROW-8314: [Python] Add a Table.select method to select a subset of columns - #7272

Closed
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select
Closed

ARROW-8314: [Python] Add a Table.select method to select a subset of columns#7272
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select

Conversation

@jorisvandenbossche

Copy link
Copy Markdown
Member

This is a pure python implementation. It might be we want that on the C++ side (unless it already exists?), but having it available in Python is already useful IMO.

@github-actions

Copy link
Copy Markdown

@nealrichardson

nealrichardson commented May 26, 2020

Copy link
Copy Markdown
Member

Here's a C++ version, albeit from the R bindings: https://github.com/apache/arrow/blob/master/r/src/table.cpp#L128-L143

Since you're doing this in Python as well, maybe this should be moved to C++?

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche Do you need help on this?

@jorisvandenbossche
jorisvandenbosscheforce-pushed the ARROW-8314-table-select branch 2 times, most recently from 940fe20 to f3175e2CompareJune 22, 2020 14:03
@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I added a C++ version (didn't yet update R to use it)

Comment threadcpp/src/arrow/table.cc Outdated

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a small type problem on the C++ side

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a static_cast, or use int64_t or size_t instead.

@pitrou

Copy link
Copy Markdown
Member

It would also be nice to add a test on the C++ side, if that's not too time-consuming.

@nealrichardson

Copy link
Copy Markdown
Member

I added a C++ version (didn't yet update R to use it)

Are you intending to make that change in this PR too?

@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I updated this, and added a small C++ test.

Are you intending to make that change in this PR too?

Realistically speaking, not at the moment (I certainly want to learn how to set up a R dev environment, but just before the release with other priorities might not be the best time ;-))
So would either merge without, unless you can quickly push a change (I suppose it might be an easy change).

@nealrichardson

Copy link
Copy Markdown
Member

I added https://issues.apache.org/jira/browse/ARROW-9387 for using this in R. It might be trivial but in case it isn't I don't want to block this.

@jorisvandenbossche

jorisvandenbossche commented Jul 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou could you check the C++ test?

(fixing the linting issue)

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche I'll take a look when I'm done with 1.0.0-critical tasks. Hopefully before the end of the week :-)

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is returning Result anyway we might as well boundscheck the indices, thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a boundscheck

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Comment threadcpp/src/arrow/table_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ASSERT_OK(subset->ValidateFull())?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

+1

@wesmwesm closed this in 1413963Jul 14, 2020
@jorisvandenbossche
jorisvandenbossche deleted the ARROW-8314-table-select branch July 14, 2020 21:04
romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Sep 7, 2020
nealrichardson added a commit that referenced this pull request Sep 11, 2020
R follow up from #7272
The current `$select()` uses a more familiar (though more expensive) tidyselect interface:
``` r
library(arrow, warn.conflicts = FALSE)
tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
```
<sup>Created on 2020-09-07 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
Do we want both ? `$select()` is used e.g. by the `read_csv(col_select=)` argument:
```r
tab <- reader$Read()$select(!!enquo(col_select))
```
Closes#8125 from romainfrancois/ARROW-9387/Table_SelectColumns
Lead-authored-by: Romain Francois <romain@rstudio.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisvandenbossche@nealrichardson@pitrou@wesm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-8314: [Python] Add a Table.select method to select a subset of columns - #7272

Closed
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select
Closed

ARROW-8314: [Python] Add a Table.select method to select a subset of columns#7272
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select

Conversation

@jorisvandenbossche

Copy link
Copy Markdown
Member

This is a pure python implementation. It might be we want that on the C++ side (unless it already exists?), but having it available in Python is already useful IMO.

@github-actions

Copy link
Copy Markdown

@nealrichardson

nealrichardson commented May 26, 2020

Copy link
Copy Markdown
Member

Here's a C++ version, albeit from the R bindings: https://github.com/apache/arrow/blob/master/r/src/table.cpp#L128-L143

Since you're doing this in Python as well, maybe this should be moved to C++?

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche Do you need help on this?

@jorisvandenbossche
jorisvandenbosscheforce-pushed the ARROW-8314-table-select branch 2 times, most recently from 940fe20 to f3175e2CompareJune 22, 2020 14:03
@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I added a C++ version (didn't yet update R to use it)

Comment threadcpp/src/arrow/table.cc Outdated

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a small type problem on the C++ side

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a static_cast, or use int64_t or size_t instead.

@pitrou

Copy link
Copy Markdown
Member

It would also be nice to add a test on the C++ side, if that's not too time-consuming.

@nealrichardson

Copy link
Copy Markdown
Member

I added a C++ version (didn't yet update R to use it)

Are you intending to make that change in this PR too?

@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I updated this, and added a small C++ test.

Are you intending to make that change in this PR too?

Realistically speaking, not at the moment (I certainly want to learn how to set up a R dev environment, but just before the release with other priorities might not be the best time ;-))
So would either merge without, unless you can quickly push a change (I suppose it might be an easy change).

@nealrichardson

Copy link
Copy Markdown
Member

I added https://issues.apache.org/jira/browse/ARROW-9387 for using this in R. It might be trivial but in case it isn't I don't want to block this.

@jorisvandenbossche

jorisvandenbossche commented Jul 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou could you check the C++ test?

(fixing the linting issue)

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche I'll take a look when I'm done with 1.0.0-critical tasks. Hopefully before the end of the week :-)

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is returning Result anyway we might as well boundscheck the indices, thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a boundscheck

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Comment threadcpp/src/arrow/table_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ASSERT_OK(subset->ValidateFull())?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

+1

@wesmwesm closed this in 1413963Jul 14, 2020
@jorisvandenbossche
jorisvandenbossche deleted the ARROW-8314-table-select branch July 14, 2020 21:04
romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Sep 7, 2020
nealrichardson added a commit that referenced this pull request Sep 11, 2020
R follow up from #7272
The current `$select()` uses a more familiar (though more expensive) tidyselect interface:
``` r
library(arrow, warn.conflicts = FALSE)
tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
```
<sup>Created on 2020-09-07 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
Do we want both ? `$select()` is used e.g. by the `read_csv(col_select=)` argument:
```r
tab <- reader$Read()$select(!!enquo(col_select))
```
Closes#8125 from romainfrancois/ARROW-9387/Table_SelectColumns
Lead-authored-by: Romain Francois <romain@rstudio.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisvandenbossche@nealrichardson@pitrou@wesm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-8314: [Python] Add a Table.select method to select a subset of columns - #7272

Closed
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select
Closed

ARROW-8314: [Python] Add a Table.select method to select a subset of columns#7272
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select

Conversation

@jorisvandenbossche

Copy link
Copy Markdown
Member

This is a pure python implementation. It might be we want that on the C++ side (unless it already exists?), but having it available in Python is already useful IMO.

@github-actions

Copy link
Copy Markdown

@nealrichardson

nealrichardson commented May 26, 2020

Copy link
Copy Markdown
Member

Here's a C++ version, albeit from the R bindings: https://github.com/apache/arrow/blob/master/r/src/table.cpp#L128-L143

Since you're doing this in Python as well, maybe this should be moved to C++?

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche Do you need help on this?

@jorisvandenbossche
jorisvandenbosscheforce-pushed the ARROW-8314-table-select branch 2 times, most recently from 940fe20 to f3175e2CompareJune 22, 2020 14:03
@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I added a C++ version (didn't yet update R to use it)

Comment threadcpp/src/arrow/table.cc Outdated

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a small type problem on the C++ side

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a static_cast, or use int64_t or size_t instead.

@pitrou

Copy link
Copy Markdown
Member

It would also be nice to add a test on the C++ side, if that's not too time-consuming.

@nealrichardson

Copy link
Copy Markdown
Member

I added a C++ version (didn't yet update R to use it)

Are you intending to make that change in this PR too?

@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I updated this, and added a small C++ test.

Are you intending to make that change in this PR too?

Realistically speaking, not at the moment (I certainly want to learn how to set up a R dev environment, but just before the release with other priorities might not be the best time ;-))
So would either merge without, unless you can quickly push a change (I suppose it might be an easy change).

@nealrichardson

Copy link
Copy Markdown
Member

I added https://issues.apache.org/jira/browse/ARROW-9387 for using this in R. It might be trivial but in case it isn't I don't want to block this.

@jorisvandenbossche

jorisvandenbossche commented Jul 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou could you check the C++ test?

(fixing the linting issue)

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche I'll take a look when I'm done with 1.0.0-critical tasks. Hopefully before the end of the week :-)

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is returning Result anyway we might as well boundscheck the indices, thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a boundscheck

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Comment threadcpp/src/arrow/table_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ASSERT_OK(subset->ValidateFull())?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

+1

@wesmwesm closed this in 1413963Jul 14, 2020
@jorisvandenbossche
jorisvandenbossche deleted the ARROW-8314-table-select branch July 14, 2020 21:04
romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Sep 7, 2020
nealrichardson added a commit that referenced this pull request Sep 11, 2020
R follow up from #7272
The current `$select()` uses a more familiar (though more expensive) tidyselect interface:
``` r
library(arrow, warn.conflicts = FALSE)
tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
```
<sup>Created on 2020-09-07 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
Do we want both ? `$select()` is used e.g. by the `read_csv(col_select=)` argument:
```r
tab <- reader$Read()$select(!!enquo(col_select))
```
Closes#8125 from romainfrancois/ARROW-9387/Table_SelectColumns
Lead-authored-by: Romain Francois <romain@rstudio.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisvandenbossche@nealrichardson@pitrou@wesm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

ARROW-8314: [Python] Add a Table.select method to select a subset of columns - #7272

Closed
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select
Closed

ARROW-8314: [Python] Add a Table.select method to select a subset of columns#7272
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select

Conversation

@jorisvandenbossche

Copy link
Copy Markdown
Member

This is a pure python implementation. It might be we want that on the C++ side (unless it already exists?), but having it available in Python is already useful IMO.

@github-actions

Copy link
Copy Markdown

@nealrichardson

nealrichardson commented May 26, 2020

Copy link
Copy Markdown
Member

Here's a C++ version, albeit from the R bindings: https://github.com/apache/arrow/blob/master/r/src/table.cpp#L128-L143

Since you're doing this in Python as well, maybe this should be moved to C++?

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche Do you need help on this?

@jorisvandenbossche
jorisvandenbosscheforce-pushed the ARROW-8314-table-select branch 2 times, most recently from 940fe20 to f3175e2CompareJune 22, 2020 14:03
@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I added a C++ version (didn't yet update R to use it)

Comment threadcpp/src/arrow/table.cc Outdated

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a small type problem on the C++ side

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a static_cast, or use int64_t or size_t instead.

@pitrou

Copy link
Copy Markdown
Member

It would also be nice to add a test on the C++ side, if that's not too time-consuming.

@nealrichardson

Copy link
Copy Markdown
Member

I added a C++ version (didn't yet update R to use it)

Are you intending to make that change in this PR too?

@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I updated this, and added a small C++ test.

Are you intending to make that change in this PR too?

Realistically speaking, not at the moment (I certainly want to learn how to set up a R dev environment, but just before the release with other priorities might not be the best time ;-))
So would either merge without, unless you can quickly push a change (I suppose it might be an easy change).

@nealrichardson

Copy link
Copy Markdown
Member

I added https://issues.apache.org/jira/browse/ARROW-9387 for using this in R. It might be trivial but in case it isn't I don't want to block this.

@jorisvandenbossche

jorisvandenbossche commented Jul 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou could you check the C++ test?

(fixing the linting issue)

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche I'll take a look when I'm done with 1.0.0-critical tasks. Hopefully before the end of the week :-)

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is returning Result anyway we might as well boundscheck the indices, thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a boundscheck

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Comment threadcpp/src/arrow/table_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ASSERT_OK(subset->ValidateFull())?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

+1

@wesmwesm closed this in 1413963Jul 14, 2020
@jorisvandenbossche
jorisvandenbossche deleted the ARROW-8314-table-select branch July 14, 2020 21:04
romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Sep 7, 2020
nealrichardson added a commit that referenced this pull request Sep 11, 2020
R follow up from #7272
The current `$select()` uses a more familiar (though more expensive) tidyselect interface:
``` r
library(arrow, warn.conflicts = FALSE)
tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
```
<sup>Created on 2020-09-07 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
Do we want both ? `$select()` is used e.g. by the `read_csv(col_select=)` argument:
```r
tab <- reader$Read()$select(!!enquo(col_select))
```
Closes#8125 from romainfrancois/ARROW-9387/Table_SelectColumns
Lead-authored-by: Romain Francois <romain@rstudio.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisvandenbossche@nealrichardson@pitrou@wesm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-8314: [Python] Add a Table.select method to select a subset of columns - #7272

Closed
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select
Closed

ARROW-8314: [Python] Add a Table.select method to select a subset of columns#7272
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select

Conversation

@jorisvandenbossche

Copy link
Copy Markdown
Member

This is a pure python implementation. It might be we want that on the C++ side (unless it already exists?), but having it available in Python is already useful IMO.

@github-actions

Copy link
Copy Markdown

@nealrichardson

nealrichardson commented May 26, 2020

Copy link
Copy Markdown
Member

Here's a C++ version, albeit from the R bindings: https://github.com/apache/arrow/blob/master/r/src/table.cpp#L128-L143

Since you're doing this in Python as well, maybe this should be moved to C++?

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche Do you need help on this?

@jorisvandenbossche
jorisvandenbosscheforce-pushed the ARROW-8314-table-select branch 2 times, most recently from 940fe20 to f3175e2CompareJune 22, 2020 14:03
@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I added a C++ version (didn't yet update R to use it)

Comment threadcpp/src/arrow/table.cc Outdated

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a small type problem on the C++ side

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a static_cast, or use int64_t or size_t instead.

@pitrou

Copy link
Copy Markdown
Member

It would also be nice to add a test on the C++ side, if that's not too time-consuming.

@nealrichardson

Copy link
Copy Markdown
Member

I added a C++ version (didn't yet update R to use it)

Are you intending to make that change in this PR too?

@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I updated this, and added a small C++ test.

Are you intending to make that change in this PR too?

Realistically speaking, not at the moment (I certainly want to learn how to set up a R dev environment, but just before the release with other priorities might not be the best time ;-))
So would either merge without, unless you can quickly push a change (I suppose it might be an easy change).

@nealrichardson

Copy link
Copy Markdown
Member

I added https://issues.apache.org/jira/browse/ARROW-9387 for using this in R. It might be trivial but in case it isn't I don't want to block this.

@jorisvandenbossche

jorisvandenbossche commented Jul 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou could you check the C++ test?

(fixing the linting issue)

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche I'll take a look when I'm done with 1.0.0-critical tasks. Hopefully before the end of the week :-)

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is returning Result anyway we might as well boundscheck the indices, thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a boundscheck

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Comment threadcpp/src/arrow/table_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ASSERT_OK(subset->ValidateFull())?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

+1

@wesmwesm closed this in 1413963Jul 14, 2020
@jorisvandenbossche
jorisvandenbossche deleted the ARROW-8314-table-select branch July 14, 2020 21:04
romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Sep 7, 2020
nealrichardson added a commit that referenced this pull request Sep 11, 2020
R follow up from #7272
The current `$select()` uses a more familiar (though more expensive) tidyselect interface:
``` r
library(arrow, warn.conflicts = FALSE)
tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
```
<sup>Created on 2020-09-07 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
Do we want both ? `$select()` is used e.g. by the `read_csv(col_select=)` argument:
```r
tab <- reader$Read()$select(!!enquo(col_select))
```
Closes#8125 from romainfrancois/ARROW-9387/Table_SelectColumns
Lead-authored-by: Romain Francois <romain@rstudio.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisvandenbossche@nealrichardson@pitrou@wesm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-8314: [Python] Add a Table.select method to select a subset of columns - #7272

Closed
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select
Closed

ARROW-8314: [Python] Add a Table.select method to select a subset of columns#7272
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select

Conversation

@jorisvandenbossche

Copy link
Copy Markdown
Member

This is a pure python implementation. It might be we want that on the C++ side (unless it already exists?), but having it available in Python is already useful IMO.

@github-actions

Copy link
Copy Markdown

@nealrichardson

nealrichardson commented May 26, 2020

Copy link
Copy Markdown
Member

Here's a C++ version, albeit from the R bindings: https://github.com/apache/arrow/blob/master/r/src/table.cpp#L128-L143

Since you're doing this in Python as well, maybe this should be moved to C++?

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche Do you need help on this?

@jorisvandenbossche
jorisvandenbosscheforce-pushed the ARROW-8314-table-select branch 2 times, most recently from 940fe20 to f3175e2CompareJune 22, 2020 14:03
@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I added a C++ version (didn't yet update R to use it)

Comment threadcpp/src/arrow/table.cc Outdated

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a small type problem on the C++ side

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a static_cast, or use int64_t or size_t instead.

@pitrou

Copy link
Copy Markdown
Member

It would also be nice to add a test on the C++ side, if that's not too time-consuming.

@nealrichardson

Copy link
Copy Markdown
Member

I added a C++ version (didn't yet update R to use it)

Are you intending to make that change in this PR too?

@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I updated this, and added a small C++ test.

Are you intending to make that change in this PR too?

Realistically speaking, not at the moment (I certainly want to learn how to set up a R dev environment, but just before the release with other priorities might not be the best time ;-))
So would either merge without, unless you can quickly push a change (I suppose it might be an easy change).

@nealrichardson

Copy link
Copy Markdown
Member

I added https://issues.apache.org/jira/browse/ARROW-9387 for using this in R. It might be trivial but in case it isn't I don't want to block this.

@jorisvandenbossche

jorisvandenbossche commented Jul 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou could you check the C++ test?

(fixing the linting issue)

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche I'll take a look when I'm done with 1.0.0-critical tasks. Hopefully before the end of the week :-)

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is returning Result anyway we might as well boundscheck the indices, thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a boundscheck

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Comment threadcpp/src/arrow/table_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ASSERT_OK(subset->ValidateFull())?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

+1

@wesmwesm closed this in 1413963Jul 14, 2020
@jorisvandenbossche
jorisvandenbossche deleted the ARROW-8314-table-select branch July 14, 2020 21:04
romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Sep 7, 2020
nealrichardson added a commit that referenced this pull request Sep 11, 2020
R follow up from #7272
The current `$select()` uses a more familiar (though more expensive) tidyselect interface:
``` r
library(arrow, warn.conflicts = FALSE)
tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
```
<sup>Created on 2020-09-07 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
Do we want both ? `$select()` is used e.g. by the `read_csv(col_select=)` argument:
```r
tab <- reader$Read()$select(!!enquo(col_select))
```
Closes#8125 from romainfrancois/ARROW-9387/Table_SelectColumns
Lead-authored-by: Romain Francois <romain@rstudio.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisvandenbossche@nealrichardson@pitrou@wesm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

ARROW-8314: [Python] Add a Table.select method to select a subset of columns - #7272

Closed
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select
Closed

ARROW-8314: [Python] Add a Table.select method to select a subset of columns#7272
jorisvandenbossche wants to merge 6 commits into
apache:masterfrom
jorisvandenbossche:ARROW-8314-table-select

Conversation

@jorisvandenbossche

Copy link
Copy Markdown
Member

This is a pure python implementation. It might be we want that on the C++ side (unless it already exists?), but having it available in Python is already useful IMO.

@github-actions

Copy link
Copy Markdown

@nealrichardson

nealrichardson commented May 26, 2020

Copy link
Copy Markdown
Member

Here's a C++ version, albeit from the R bindings: https://github.com/apache/arrow/blob/master/r/src/table.cpp#L128-L143

Since you're doing this in Python as well, maybe this should be moved to C++?

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche Do you need help on this?

@jorisvandenbossche
jorisvandenbosscheforce-pushed the ARROW-8314-table-select branch 2 times, most recently from 940fe20 to f3175e2CompareJune 22, 2020 14:03
@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I added a C++ version (didn't yet update R to use it)

Comment threadcpp/src/arrow/table.cc Outdated

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a small type problem on the C++ side

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a static_cast, or use int64_t or size_t instead.

@pitrou

Copy link
Copy Markdown
Member

It would also be nice to add a test on the C++ side, if that's not too time-consuming.

@nealrichardson

Copy link
Copy Markdown
Member

I added a C++ version (didn't yet update R to use it)

Are you intending to make that change in this PR too?

@jorisvandenbossche

Copy link
Copy Markdown
MemberAuthor

I updated this, and added a small C++ test.

Are you intending to make that change in this PR too?

Realistically speaking, not at the moment (I certainly want to learn how to set up a R dev environment, but just before the release with other priorities might not be the best time ;-))
So would either merge without, unless you can quickly push a change (I suppose it might be an easy change).

@nealrichardson

Copy link
Copy Markdown
Member

I added https://issues.apache.org/jira/browse/ARROW-9387 for using this in R. It might be trivial but in case it isn't I don't want to block this.

@jorisvandenbossche

jorisvandenbossche commented Jul 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou could you check the C++ test?

(fixing the linting issue)

@pitrou

Copy link
Copy Markdown
Member

@jorisvandenbossche I'll take a look when I'm done with 1.0.0-critical tasks. Hopefully before the end of the week :-)

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is returning Result anyway we might as well boundscheck the indices, thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a boundscheck

Comment threadcpp/src/arrow/table.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1.

Comment threadcpp/src/arrow/table_test.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ASSERT_OK(subset->ValidateFull())?

@wesm

wesm commented Jul 14, 2020

Copy link
Copy Markdown
Member

+1

@wesmwesm closed this in 1413963Jul 14, 2020
@jorisvandenbossche
jorisvandenbossche deleted the ARROW-8314-table-select branch July 14, 2020 21:04
romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Sep 7, 2020
nealrichardson added a commit that referenced this pull request Sep 11, 2020
R follow up from #7272
The current `$select()` uses a more familiar (though more expensive) tidyselect interface:
``` r
library(arrow, warn.conflicts = FALSE)
tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>
```
<sup>Created on 2020-09-07 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
Do we want both ? `$select()` is used e.g. by the `read_csv(col_select=)` argument:
```r
tab <- reader$Read()$select(!!enquo(col_select))
```
Closes#8125 from romainfrancois/ARROW-9387/Table_SelectColumns
Lead-authored-by: Romain Francois <romain@rstudio.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jorisvandenbossche@nealrichardson@pitrou@wesm