Fixes #496: support for joinCollections #500

pikatchu · 2024-11-12T15:23:37Z

This diff implements the 'join' operation of two collections. It is assumed that:

both collection values are objects (keys can be of any type)
there never is more than 1 object on boths sides at any time unless the option allowMultipleValuesOnBothSides is passed (which should be rare).

If both those conditions are met, given a key, if the first collection defines an object of type V1 and the second collection defines an object V2 for the same key, the resulting collection will associate {...V1, ...V2} for that key. No value is associated if either side is missing.

bennostein · 2024-11-12T15:26:27Z

skipruntime-ts/helpers/src/utils.ts

+}
+
+class MergeJoinFields implements Mapper<JoinObject, JoinObject, Json, Json> {
+  constructor(private allowNN: boolean) {}


Hmm, I can't figure out what "NN" stands for here -- if it's meant to be rare, shall we just expand that abbreviation?

It's for N to N relations. But I renamed it to allowMultipleValuesOnBothSides.

bennostein · 2024-11-12T15:30:50Z

skipruntime-ts/helpers/src/utils.ts

+  implements Mapper<K, V, K, V>
+{
+  mapEntry(key: K, values: NonEmptyIterator<V>): Iterable<[K, V]> {
+    return [[key, values.getUnique()]];


Is this ever not either a no-op or an exception?

I'm not sure that the benefit of having a special UniqueEagerCollection type is worth the extra API surface and copying/allocation (though I suppose that will be minimal here due to sharing).

I'd propose either (a) checking uniqueness during the actual join operation and dropping this stuff or (b) making UniqueEagerCollection a more full-fledged component of the API, with its own getter/mapper types that take advantage of the uniqueness. Probably slightly prefer (a) for simplicity's sake, but open to arguments / the possibility that I'm misunderstanding something here.

Ok. I removed it. The join doesn't need it to operate, so no change needed there.

jberdine · 2024-11-12T20:33:31Z

skipruntime-ts/helpers/src/utils.ts

+// Joins
+/*****************************************************************************/
+
+type JoinObject = { value: Json; side: "left" | "right" };


Would JsonObject instead of Json here be more accurate and simplify the code and interface?

Let me try (I thought I tried to add JsonObject and it didn't work, but it might have been somewhere else).

So yeah, typescript refuses it. I don't understand why ...

I tried and pushed the result to your branch. Seems to work, or am I missing something?

So I see you pushed a diff that makes JsonObject bubble up all the way to joinCollections. I don't know that this is a good idea.

The reason is that when I tried to use joinCollections on inputs, Typescript did not complain even when the input collections where not objects. That is a pretty subtle "gotcha" that I would like to avoid.

jberdine · 2024-11-12T20:44:35Z

skipruntime-ts/helpers/src/utils.ts

+class AddJoinSideField implements Mapper<Json, Json, Json, JoinObject> {
+  constructor(private joinSide: "left" | "right") {}
+
+  mapEntry(
+    key: Json,
+    values: NonEmptyIterator<Json>,
+  ): Iterable<[Json, JoinObject]> {
+    const result: [Json, JoinObject][] = [];
+    for (const v of values) {
+      if (typeof v !== "object") {
+        throw new Error(
+          "joinCollection only works on objects, not: " + JSON.stringify(v),
+        );
+      }
+      result.push([key, { value: v, side: this.joinSide }]);
+    }
+    return result;
+  }
+}


Am I missing something or could this be simplified by extending OneToOneMapper instead of implementing Mapper?

Good point.

jberdine · 2024-11-12T20:48:42Z

skipruntime-ts/helpers/src/utils.ts

+    );
+  }
+  const keys1 = Object.keys(v1);
+  if (keys1.some((key) => key in v2)) {


Are these objects expected to be small, or might they be dictionaries or arrays? Asking since this is quadratic and doesn't strictly need to be.

Ah. That's a very good point. Let me revisit.

But the objects are indeed expected to be small ... still, no need to keep something O(n^2) for no reason.

Annoyingly, Object.keys is not sorted, so they would have to be first sorted and then zipped together a la mergesort.

jberdine · 2024-11-12T21:32:52Z

skipruntime-ts/helpers/src/utils.ts

+>(
+  col1: EagerCollection<K, V1>,
+  col2: EagerCollection<K, V2>,
+  allowMultipleValuesOnBothSides: boolean = false,


The expectation is that the fields of the values in the left and right value types are disjoint, right? Or is it expected that idiomatic use cases will have non-disjoint types, but only per-key will disjointness will hold?

My understanding is that the simplest case is where both the left and right argument collections map keys to unique values. On the other hand, if the left and right argument collections map keys to potentially multiple values, then the result will map each key to the merge of each pair in the cartesian product of the left values and right values for that key.

Maybe this is just me, but I don't see why the intermediate cases, where only one of the argument collections is multi-valued, are grouped with the simple case rather than the general one. Would it be a simpler interface if there were two booleans, one stating that the left collection is allowed to be multi-valued and the other stating that the right collection is allowed to be multi-valued?

The reason is my experience with the SQL engine (which might be flawed). I have seen multiple cases where either side had more than one value, but never something that required a cartesian product that was actually useful. Let's review them, let's assume we have 2 large collections (large in the sense that N*M is too large to be computed).

Example, one / many: A table of users, a table of addresses, a user can have multiple addresses => maximum size of the able = number of address
Example, one / one: A table of users, a table of social security numbers => maximum size of the table = min(users, ssn)
Example, many / many: ??? Usually a contrived example that is only useful to explain the behavior of a join

I understand the cartesian product in the many/many case is the natural extension of what the join should do. But I have yet to see anything in practice using it. In fact, more often than not, it's a mistake, the query explodes and the user does not know why.

When you think about it, it makes sense. A cartesian product is only going to work for very small sets ...

So I would rather make it explicit. It's not banned. You just have to be explicit about it.

jberdine · 2024-11-12T21:35:32Z

skipruntime-ts/tests/src/tests.ts

+    input1: UniqueEagerCollection<number, JsonObject>;
+    input2: EagerCollection<number, JsonObject>;
+  }): EagerCollection<number, JsonObject> {
+    return joinCollections(cs.input1, cs.input2);


Add a TODO to add tests for the 1_to_N, N_to_1, and N_to_N cases?

jberdine · 2024-11-12T21:40:27Z

skipruntime-ts/tests/src/tests.ts

+import {
+  Sum,
+  joinCollections,
+  type UniqueEagerCollection,


jberdine · 2024-11-13T11:34:50Z

Summarizing offline discussion: It would be possible to give joinCollections the type

export function joinCollections<
  K extends Json,
  V1 extends JsonObject,
  V2 extends JsonObject,
>(
  col1: EagerCollection<K, V1>,
  col2: EagerCollection<K, V2>,
  allowMultipleValuesOnBothSides: boolean = false,
): EagerCollection<K, V1 & V2>

and propagate JsonObject down, eliminating the checks that each value is an object.

BUT, since typescript unsoundly treats method parameters as bivariant, calling joinCollections e.g. from createGraph where inputCollections: Record<string, EagerCollection<Json, Json>> will not detect mistakes where general Json values are provided where JsonObjects are required. So this PR's dynamic checks on each value are still needed. Thanks for nothing typescript.

jberdine · 2024-11-13T12:09:47Z

skipruntime-ts/helpers/src/utils.ts

+    throw new Error(
+      "Objects don't have distinct fields: " +
+        JSON.stringify(v1) +
+        " " +
+        JSON.stringify(v2),
+    );


A question on the error behavior: join in relational algebra drops pairs that do not agree on common fields, while here it is an error for there to be common fields (other than the key). No strong position, but it isn't immediately obvious to me that this choice is the one users will expect, or be the most useful.

To clarify, I wonder about just dropping the pairs with common fields other than the key, rather than throwing, not taking pairs that happen to have matching values for common fields.

I don't know people know relational algebra well enough to expect that. I suspect they don't.
I am happy to add an option (dropOnCommonFields for example), but making that the default is not a good idea IMO. Imagine that is not the behavior you expect, now imagine you end up inserting new kinds of objects in a collection with a field that collides with another collection by mistake. The debugging is going to be pretty brutal ...

Ok makes sense, the debugging angle is compelling. And we can wait to see if anyone asks to drop instead of error.

jberdine · 2024-11-13T12:11:32Z

skipruntime-ts/helpers/src/utils.ts

+  }
+}
+
+export function joinCollections<


From an implementation point of view, I understand why this function is here. But from the API point of view, I think it would be better if this was a join operation in the EagerCollection interface. Also, if/when we reimplement this in skip, putting it in helpers will not be the right place anymore.

You are right. I wish there was a clean way of extending a class in another file. You know something like:

extension class EagerCollection { join(...): ... }

- AddJoinSideField now uses OneToOneMapper - Check of distinct fields is now O(n*log(n)) - Added tests for one to many and many to one use case

jberdine · 2024-11-13T12:34:33Z

skipruntime-ts/helpers/src/utils.ts

+      }
+    }
+    if (countLeft > 1 && countRight > 1) {
+      if (!this.allowMultipleValuesOnBothSides) {


This could move up to before let countLeft... to avoid the counting loop in the allow case.

It's not the common case, so I think it's fine. If allowMultipleValuesOnBothSides is true, your set better be very small to begin with ...

skipruntime-ts/helpers/src/utils.ts

jberdine · 2024-11-13T12:37:42Z

skipruntime-ts/helpers/src/utils.ts

+  V1 extends Json,
+  V2 extends Json,


Even if TS doesn't check it, I think it would be a documentation improvement to use JsonObject for the value types here.

Agreed. Done.

This diff implements the 'join' operation of two collections. It is assumed that: - both collection values are objects (keys can be of any type) - there never is more than 1 object on boths sides at any time unless the option allowNN is passed (which should be rare). If both those conditions are met, given a key, if the first collection defines an object of type V1 and the second collection defines an object V2 for the same key, the resulting collection will associate {...V1, ...V2} for that key. No value is associated if either side is missing.

- AddJoinSideField now uses OneToOneMapper - Check of distinct fields is now O(n*log(n)) - Added tests for one to many and many to one use case

Co-authored-by: Josh Berdine <[email protected]>

pikatchu requested a review from jberdine November 12, 2024 15:23

bennostein reviewed Nov 12, 2024

View reviewed changes

jberdine reviewed Nov 12, 2024

View reviewed changes

skipruntime-ts/tests/src/tests.ts Outdated

import {

Sum,

joinCollections,

type UniqueEagerCollection,

Copy link

Contributor

jberdine Nov 12, 2024

Choose a reason for hiding this comment

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

gone now

jberdine reviewed Nov 13, 2024

View reviewed changes

pikatchu added a commit to pikatchu/skdb that referenced this pull request Nov 13, 2024

Addressed comments from PR SkipLabs#500

e6a15ea

- AddJoinSideField now uses OneToOneMapper - Check of distinct fields is now O(n*log(n)) - Added tests for one to many and many to one use case

pikatchu force-pushed the join branch from a917a03 to e6a15ea Compare November 13, 2024 12:31

jberdine reviewed Nov 13, 2024

View reviewed changes

pikatchu and others added 8 commits November 14, 2024 16:53

Fixed eslint errors

87edbe1

Removed UniqueEagerCollection type

a23585e

Renamed allowNN to allowMultipleValuesOnBothSides

7e49ff4

Addressed comments from PR SkipLabs#500

17d143f

- AddJoinSideField now uses OneToOneMapper - Check of distinct fields is now O(n*log(n)) - Added tests for one to many and many to one use case

Update skipruntime-ts/helpers/src/utils.ts

f473a67

Co-authored-by: Josh Berdine <[email protected]>

Changed the types in joinCollections to JsonObject (instead of Json)

e27e83a

Adapted unit test since the error message changed

c79181e

pikatchu force-pushed the join branch from 16254da to c79181e Compare November 14, 2024 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #496: support for joinCollections #500

Fixes #496: support for joinCollections #500

pikatchu commented Nov 12, 2024 •

edited

Loading

bennostein Nov 12, 2024

pikatchu Nov 12, 2024

bennostein Nov 12, 2024

pikatchu Nov 12, 2024

jberdine Nov 12, 2024

pikatchu Nov 13, 2024

pikatchu Nov 13, 2024

jberdine Nov 13, 2024

pikatchu Nov 13, 2024

jberdine Nov 12, 2024

pikatchu Nov 13, 2024

jberdine Nov 12, 2024

pikatchu Nov 13, 2024

pikatchu Nov 13, 2024

jberdine Nov 13, 2024

jberdine Nov 12, 2024

pikatchu Nov 13, 2024

jberdine Nov 12, 2024

jberdine Nov 12, 2024

jberdine commented Nov 13, 2024

jberdine Nov 13, 2024

jberdine Nov 13, 2024

pikatchu Nov 13, 2024

jberdine Nov 13, 2024

jberdine Nov 13, 2024

pikatchu Nov 13, 2024

jberdine Nov 13, 2024

pikatchu Nov 13, 2024

jberdine Nov 13, 2024

pikatchu Nov 13, 2024

Fixes #496: support for joinCollections #500

Are you sure you want to change the base?

Fixes #496: support for joinCollections #500

Conversation

pikatchu commented Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberdine commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pikatchu commented Nov 12, 2024 •

edited

Loading