Re: [PR] fix: use total ordering in the min & max accumulator for floats [datafusion]

2024-05-25 Thread via GitHub
alamb commented on PR #10627: URL: https://github.com/apache/datafusion/pull/10627#issuecomment-2131171739 > If this is the case then there is divergence between postgres and arrow-rs. Which takes priority? I would personally suggest we do whatever is consistent with arrow (and easie

Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]

2024-05-25 Thread via GitHub
peter-toth commented on code in PR #10543: URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614522462 ## datafusion/physical-plan/src/work_table.rs: ## @@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec { &self.cache } -fn children(&sel

Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]

2024-05-25 Thread via GitHub
peter-toth commented on code in PR #10543: URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614522462 ## datafusion/physical-plan/src/work_table.rs: ## @@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec { &self.cache } -fn children(&sel

[PR] Fix typo in Cargo.toml [datafusion]

2024-05-25 Thread via GitHub
alamb opened a new pull request, #10662: URL: https://github.com/apache/datafusion/pull/10662 ## Which issue does this PR close? Closes #. ## Rationale for this change When running `cargo build` locally I see ``` warning: /Users/andrewlamb/Software/data

Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]

2024-05-25 Thread via GitHub
peter-toth commented on code in PR #10543: URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614522462 ## datafusion/physical-plan/src/work_table.rs: ## @@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec { &self.cache } -fn children(&sel

Re: [PR] feat: add hex scalar function [datafusion-comet]

2024-05-25 Thread via GitHub
advancedxy commented on code in PR #449: URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1614516643 ## core/src/execution/datafusion/expressions/scalar_funcs/hex.rs: ## @@ -0,0 +1,371 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or m

Re: [PR] Fix incorrect statistics read for binary columns in parquet [datafusion]

2024-05-25 Thread via GitHub
alamb commented on PR #10645: URL: https://github.com/apache/datafusion/pull/10645#issuecomment-2131168957 > The gate issue is not related to the PR.. could we rerun? I restarted it FYI another trick we have found is if you close the PR and then reopen it, github will rerun al

Re: [I] [Epic] Unify `AggregateFunction` Interface (remove built in list of `AggregateFunction` s), improve the system [datafusion]

2024-05-25 Thread via GitHub
alamb commented on issue #8708: URL: https://github.com/apache/datafusion/issues/8708#issuecomment-2131167747 After https://github.com/apache/datafusion/pull/10648 and https://github.com/apache/datafusion/issues/10389 I think we have a pretty good set of examples of how to move aggregates o

[PR] fix: substring with negative indices should produce correct result [datafusion-comet]

2024-05-25 Thread via GitHub
sonhmai opened a new pull request, #470: URL: https://github.com/apache/datafusion-comet/pull/470 ## Which issue does this PR close? Closes #463 ## Rationale for this change Fix for substring with negative indices produces incorrect results ## What changes are

Re: [PR] Convert first, last aggregate function to UDAF [datafusion]

2024-05-25 Thread via GitHub
alamb commented on code in PR #10648: URL: https://github.com/apache/datafusion/pull/10648#discussion_r1614514043 ## datafusion/physical-expr-common/src/utils.rs: ## @@ -100,15 +103,37 @@ pub fn reverse_order_bys(order_bys: &[PhysicalSortExpr]) -> Vec`. +/// If conversion is no

Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]

2024-05-25 Thread via GitHub
peter-toth commented on code in PR #10543: URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614503459 ## datafusion/physical-plan/src/work_table.rs: ## @@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec { &self.cache } -fn children(&sel

Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]

2024-05-25 Thread via GitHub
peter-toth commented on code in PR #10543: URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614503459 ## datafusion/physical-plan/src/work_table.rs: ## @@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec { &self.cache } -fn children(&sel

Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]

2024-05-25 Thread via GitHub
peter-toth commented on code in PR #10543: URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614503459 ## datafusion/physical-plan/src/work_table.rs: ## @@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec { &self.cache } -fn children(&sel

[PR] Remove duplicate function name in its aliases list [datafusion]

2024-05-25 Thread via GitHub
goldmedal opened a new pull request, #10661: URL: https://github.com/apache/datafusion/pull/10661 ## Which issue does this PR close? Closes #10658 ## Rationale for this change ## What changes are included in this PR? ## Are these changes te

Re: [PR] Pass BigQuery options to the ArrowSchema [datafusion]

2024-05-25 Thread via GitHub
ozankabak commented on PR #10590: URL: https://github.com/apache/datafusion/pull/10590#issuecomment-2131154079 The aim has been to make the `COPY` and `CREATE EXTERNAL TABLE` statements use the same `OPTIONS` syntax. The PR you mention was actually a part of multi-PR effort to make this so

Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children()` and `PhysicalExpr::children()` return references [datafusion]

2024-05-25 Thread via GitHub
ozankabak commented on code in PR #10543: URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614484172 ## datafusion/physical-plan/src/work_table.rs: ## @@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec { &self.cache } -fn children(&self

Re: [PR] Add reference visitor `TreeNode` APIs, change `ExecutionPlan::children` to return references [datafusion]

2024-05-25 Thread via GitHub
peter-toth commented on code in PR #10543: URL: https://github.com/apache/datafusion/pull/10543#discussion_r1614462365 ## datafusion/physical-plan/src/work_table.rs: ## @@ -169,7 +169,7 @@ impl ExecutionPlan for WorkTableExec { &self.cache } -fn children(&sel

Re: [PR] Minor: Csv Options Clean-up [datafusion]

2024-05-25 Thread via GitHub
ozankabak commented on PR #10650: URL: https://github.com/apache/datafusion/pull/10650#issuecomment-2131118238 Maybe I can help clarify things a little bit. The purpose of the PR is not to change the current fallback behavior, just simplify its implementation. AFAICT @berkaysynnada is movin

Re: [PR] Minor: Csv Options Clean-up [datafusion]

2024-05-25 Thread via GitHub
ozankabak commented on code in PR #10650: URL: https://github.com/apache/datafusion/pull/10650#discussion_r1614446423 ## datafusion/physical-expr/src/expressions/cast.rs: ## @@ -170,7 +170,8 @@ impl PhysicalExpr for CastExpr { let target_type = &self.cast_type;

<    1   2