To be clear given the scope of code affected I think we should merge it today and address further feedback in a follow up patch. I will be diligent about responding to additional comments in the PR
On Sat, May 23, 2020, 3:19 PM Wes McKinney <wesmck...@gmail.com> wrote: > Yes you should still be able to comment. I will reopen the PR after it is > merged > > On Sat, May 23, 2020, 2:52 PM Micah Kornfield <emkornfi...@gmail.com> > wrote: > >> Hi Wes, >> Will we still be able to comment on the PR once it is closed? >> >> >> If we want to be inclusive on feedback it might pay to wait until Tuesday >> evening US time to merge since it is a long weekend here. >> >> Thanks, >> Micah >> >> On Saturday, May 23, 2020, Wes McKinney <wesmck...@gmail.com> wrote: >> >>> Hi folks -- I've addressed a good deal of feedback and added a lot of >>> comments and with Kou's help have got the build passing, It would be >>> great if this could be merged soon to unblock follow up PRs >>> >>> On Wed, May 20, 2020 at 11:55 PM Wes McKinney <wesmck...@gmail.com> >>> wrote: >>> > >>> > I just opened the PR https://github.com/apache/arrow/pull/7240 >>> > >>> > I'm sorry it's so big. I really think this is the best way. The only >>> > further work I plan to do on it is to get the CI passing. >>> > >>> > On Wed, May 20, 2020 at 12:26 PM Wes McKinney <wesmck...@gmail.com> >>> wrote: >>> > > >>> > > I'd guess I'm < 24 hours away from putting up my initial PR for this >>> > > work. Since the work is large and (for all practical purposes) nearly >>> > > impossible to separate into separately merge-ready PRs, I'll start a >>> > > new e-mail thread describing what I've done in more detail and >>> > > proposing a path for merging the PR (so as to unblock PRs into >>> > > arrow/compute and avoid rebasing headaches) and addressing review >>> > > feedback that will likely take several weeks to fully accumulate. >>> > > >>> > > On Mon, May 11, 2020 at 5:17 PM Wes McKinney <wesmck...@gmail.com> >>> wrote: >>> > > > >>> > > > I'm working actively on this but perhaps as expected it has >>> ballooned >>> > > > into a very large project -- it's unclear at the moment whether >>> I'll >>> > > > be able to break the work into smaller patches that are easier to >>> > > > digest. I'm working as fast as I can to have an initial >>> > > > feature-preserving PR up, but the changes to the src/arrow/compute >>> > > > directory are extensive, so I would please ask that we do not merge >>> > > > non-essential patches into cpp/src/arrow/compute until I get the >>> PR up >>> > > > (estimated later this week at present rate) so we can see where >>> things >>> > > > stand. >>> > > > >>> > > > On Wed, Apr 22, 2020 at 8:06 AM Wes McKinney <wesmck...@gmail.com> >>> wrote: >>> > > > > >>> > > > > On Wed, Apr 22, 2020 at 12:41 AM Micah Kornfield < >>> emkornfi...@gmail.com> wrote: >>> > > > > > >>> > > > > > Hi Wes, >>> > > > > > I haven't had time to read the doc, but wanted to ask some >>> questions on >>> > > > > > points raised on the thread. >>> > > > > > >>> > > > > > * For efficiency, kernels used for array-expr evaluation >>> should write >>> > > > > > > into preallocated memory as their default mode. This enables >>> the >>> > > > > > > interpreter to avoid temporary memory allocations and >>> improve CPU >>> > > > > > > cache utilization. Almost none of our kernels are >>> implemented this way >>> > > > > > > currently. >>> > > > > > >>> > > > > > Did something change, I was pretty sure I submitted a patch a >>> while ago for >>> > > > > > boolean kernels, that separated out memory allocation from >>> computation. >>> > > > > > Which should allow for writing to the same memory. Is this a >>> concern with >>> > > > > > the public Function APIs for the Kernel APIs themselves, or a >>> lower level >>> > > > > > implementation concern? >>> > > > > >>> > > > > Yes, you did in the internal implementation [1]. The concern is >>> the >>> > > > > public API and the general approach to implementing new kernels. >>> > > > > >>> > > > > I'm working on this right now (it's a large project so it will >>> take me >>> > > > > a little while to produce something to be reviewed) so bear with >>> me =) >>> > > > > >>> > > > > [1]: >>> https://github.com/apache/arrow/commit/4910fbf4fda05b864daaba820db08291e4afdcb6#diff-561ea05d36150eb15842f452e3f07c76 >>> > > > > >>> > > > > > * Sorting is generally handled by different data processing >>> nodes from >>> > > > > > > Projections, Aggregations / Hash Aggregations, Filters, and >>> Joins. >>> > > > > > > Projections and Filters use expressions, they do not sort. >>> > > > > > >>> > > > > > Would sorting the list-column elements per row be an >>> array-expr? >>> > > > > >>> > > > > Yes, as that's an element-wise function. When I said sorting I >>> was >>> > > > > referring to ORDER BY. The functions we have that do sorting do >>> so in >>> > > > > the context of a single array [2]. >>> > > > > >>> > > > > A query engine must be able to sort a (potentially very large) >>> stream >>> > > > > of record batches. One approach is for the Sort operator to >>> exhaust >>> > > > > its child input, accumulating all of the record batches in memory >>> > > > > (spilling to disk as needed) and then sorting and emitting record >>> > > > > batches from the sorted records/tuples. See e.g. Impala's >>> sorting code >>> > > > > [3] [4] >>> > > > > >>> > > > > [2]: >>> https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/sort_to_indices.h#L34 >>> > > > > [3]: >>> https://github.com/apache/impala/blob/master/be/src/runtime/sorter.h >>> > > > > [4]: >>> https://github.com/apache/impala/blob/master/be/src/exec/sort-node.h >>> > > > > >>> > > > > > >>> > > > > > On Tue, Apr 21, 2020 at 5:35 AM Wes McKinney < >>> wesmck...@gmail.com> wrote: >>> > > > > > >>> > > > > > > On Tue, Apr 21, 2020 at 7:32 AM Antoine Pitrou < >>> anto...@python.org> wrote: >>> > > > > > > > >>> > > > > > > > >>> > > > > > > > Le 21/04/2020 à 13:53, Wes McKinney a écrit : >>> > > > > > > > >> >>> > > > > > > > >> That said, in the SortToIndices case, this wouldn't be >>> a problem, >>> > > > > > > since >>> > > > > > > > >> only the second pass writes to the output. >>> > > > > > > > > >>> > > > > > > > > This kernel is not valid for normal array-exprs (see the >>> spreadsheet I >>> > > > > > > > > linked), such as what you can write in SQL >>> > > > > > > > > >>> > > > > > > > > Kernels like SortToIndices are a different type of >>> function (in other >>> > > > > > > > > words, "not a SQL function") and so if we choose to >>> allow such a >>> > > > > > > > > "non-SQL-like" functions in the expression evaluator >>> then different >>> > > > > > > > > logic must be used. >>> > > > > > > > >>> > > > > > > > Hmm, I think that maybe I'm misunderstanding at which >>> level we're >>> > > > > > > > talking here. SortToIndices() may not be a "SQL >>> function", but it looks >>> > > > > > > > like an important basic block for a query engine (since, >>> after all, >>> > > > > > > > sorting results is an often used feature in SQL and other >>> languages). >>> > > > > > > > So it should be usable *inside* the expression engine, >>> even though it's >>> > > > > > > > not part of the exposed vocabulary, no? >>> > > > > > > >>> > > > > > > No, not as part of "expressions" as they are defined in the >>> context of >>> > > > > > > SQL engines. >>> > > > > > > >>> > > > > > > Sorting is generally handled by different data processing >>> nodes from >>> > > > > > > Projections, Aggregations / Hash Aggregations, Filters, and >>> Joins. >>> > > > > > > Projections and Filters use expressions, they do not sort. >>> > > > > > > >>> > > > > > > > Regards >>> > > > > > > > >>> > > > > > > > Antoine. >>> > > > > > > >>> >>
