-
Notifications
You must be signed in to change notification settings - Fork 3
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
implement a reduce
operator & ZQL group-by on top of it
#45
Conversation
note to self: |
4ad041e
to
de48ae2
Compare
if (entries === undefined) { | ||
continue; | ||
} | ||
this.#index.delete(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the iteration order of the keys. Is that intentional? I think it would be simpler to only delete if length === 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not intentional but also shouldn't matter?
Updating to only delete on length === 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, slightly more efficient because less tombstones on the map storage ;-)
de48ae2
to
98a1c34
Compare
reduce
operator to support group-byreduce
& group-by
Not sure that I like the aggregation API: q
.select('status')
.groupBy('status')
.array('assignee')
.min('created'); maybe it should be: q
.select('status', agg.array('assignee'), agg.min('created'))
.groupBy('status') |
reduce
& group-byreduce
operator & ZQL group-by on top of it
Another strawman: q
.select('status').
.agg('array', 'assignee')
.agg('min', 'created') But I think I prefer the first suggested API. |
src/zql/query/entity-query.ts
Outdated
constructor(context: Context, tableName: string, ast?: AST) { | ||
this.#ast = ast ?? { | ||
table: tableName, | ||
alias: aliasCount++, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the alias used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'll be used whenever we compile the query to SQL.
I think I mentioned this before but since the same table could be present many times (through joins and sub-queries) it'll be simplest to give each occurrence a unique alias.
Maybe it is irrelevant to do at this step and the AST -> SQL compiler can do this itself.
98a1c34
to
58c54f6
Compare
new API: ```ts q.select('foo', agg.min('bar'), agg.array('baz')).groupBy('foo') ```
updated the API to allow putting aggregate calls in q
.select(
'status',
agg.array('assignee'),
agg.min('created', 'minCreated'),
agg.max('created', 'maxCreated'),
) |
we could eventually support this.
@@ -5,6 +5,18 @@ | |||
// input to the query builder. | |||
export type Ordering = readonly [readonly string[], 'asc' | 'desc']; | |||
export type Primitive = string | number | boolean | null; | |||
|
|||
// I think letting users provide their own lambda functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That introduces the possibilities of exceptions in the pipeline
It also makes converting to SQL impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the lambdas would only be available client side. What is run server side would have to return a superset of the data.
src/zql/query/entity-query.ts
Outdated
}); | ||
} | ||
|
||
groupBy<K extends keyof S['fields']>(...x: K[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
groupBy<K extends keyof S['fields']>(...x: K[]) { | |
groupBy<K extends Selectable<S>>(...x: K[]) { |
- type alias on `Aggregate< AsString<keyof S['fields']>, string >;` - aggrable - aggregable - remove operator-index - test flatMapIter - tree-shakable `agg` exports - remove unused await
* Flat maps the items returned from the iterable. | ||
* | ||
* `iter` is a lambda that returns an iterable | ||
* so this function can return an `IterableIterator` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should take an IterableIterator then?
Also, These should be compatible with the standard methods to simplify transition to them later:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator/flatMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to iterate on this separately. IterableIterator
is also getting exhausted on me :/
E.g.,
x = flatMapIter(iter, fn);
console.log([...x])
console.log([...x])
second spread is empty :/
Or maybe a better question to answer for myself is why would we need to pull the iterable twice. Seems like it should only ever happen once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get the second log to be non empty x
needs to be something that has [Symbol.iterator](): Iterator
which returns a new iterator every time.
This API was something that I felt we didn't get quite right for ES6. It is always confusing.
All aggregate functions can be modeled as
reduce
This adds a
reduce
operator so we can do:This targets SQL behavior for group-by.