Add support for `Is_In` and `Not_In` to `Filter_Condition` #3790

radeusgd · 2022-10-11T21:38:07Z

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/183389945

Important Notes

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

radeusgd · 2022-10-12T12:57:01Z

build.sbt

@@ -1190,7 +1190,6 @@ lazy val parser = (project in file("lib/scala/parser"))
      s"-Djava.library.path=$root/target/rust/debug"
    },
    libraryDependencies ++= Seq(
-      "com.storm-enroute" %% "scalameter" % scalameterVersion % "bench",


I don't think this parser project is used at all, I think it was a prototype of the Rust integration, superseded by the new Rust parser. Can we just remove the whole project? cc: @hubertp @jdunkerley

I removed this line in particular as bench config is not available for this project and cleaning it didn't work because of that. But if possible maybe we should remove it altogether if it is indeed unused.

Looks like this can be removed. But maybe @JaroslavTulach can confirm since he is tinkering with the integration

If it builds after removing the line feel free to remove it.

The line yep, I was meaning about removing the whole subproject - it seems to me it's some old Rust parser integration effort (it's not the old parser still in use today, that's syntax) - which was superseded by yours effort and I don't think it is used anywhere. So I think we should ditch it altogether to avoid the maintenance.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Filter_Condition.enso

radeusgd · 2022-10-12T13:50:29Z

test/Table_Tests/src/Table_Spec.enso

+            expected_max_time_ms = init.first.total_milliseconds * 2
+            check_timing name ~action =
+                res = Duration.time_execution action
+                runtime_ms = res.first.total_milliseconds
+                if runtime_ms > expected_max_time_ms then
+                    Test.fail "Expected `Is_In` on "+name+" to be efficient, but it took "+runtime_ms.to_text+"ms while initialization itself took just "+expected_max_time_ms.to_text+"ms."
+
+            check_timing "integers" <|
+                t.filter "X" (Filter_Condition.Is_In vec) . at "X" . to_vector . should_equal expected_vector
+
+            check_timing "booleans" <|
+                t.filter (t.at "X" % 2 == 0) (Filter_Condition.Is_In bool_vec) . at "X" . to_vector . should_equal expected_vector_2
+
+            check_timing "dates" <|
+                t.filter date_col (Filter_Condition.Is_In dates_vec) . at "X" . to_vector . should_equal expected_vector


I did a timing check just to have a decent comparison and:

init = 501 integers = 31 booleans = 28 dates = 35

so init*2 seems to still have a decent buffer.

For example if I disable the vectorized variant for booleans it gives:

- [FAILED] should perform `Is_In` efficiently for builtin types [14626ms] Reason: Expected `Is_In` on booleans to be efficient, but it took 13852ms while initialization itself took just 1472ms.

So we can see that 2*init is a decent buffer - for good implementation it gives enough time, but the wrong implementation exceeds that greatly anyway. Let's see how it behaves on the CI though.

jdunkerley

Nothing major a few comments to improve but looks good.

distribution/lib/Standard/Table/0.0.0-dev/src/Data/Column.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Filter_Condition.enso

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Base_Generator.enso

std-bits/table/src/main/java/org/enso/table/data/column/operation/map/bool/BooleanIsInOp.java

jdunkerley · 2022-10-13T09:02:02Z

std-bits/table/src/main/java/org/enso/table/data/column/operation/map/SpecializedIsInOp.java

+   * <p>It indicates whether the vector contained a null value and contains a hashmap of the vector
+   * elements for faster contains checks.
+   */
+  public record CompactRepresentation(HashSet<Object> coercedValues, boolean hasNulls) {}


Could we do a generic version? I don't think will make any real difference but wondered if nicer to have HashSet<String> etc for example.

I tried. It was a bit more complex than we'd like, but maybe the changes could open venues for writing more type-aware operations in the future.

I first made the SpecializedIsInOp generic as asked ( 2430d46 ), to do so I needed to introduce some type markers to make stuff compile.

Then in 5eb9c3f I have removed the temporarily added TypedStorage<T> marker interface and just made Storage know its type Storage<T>. This required some more work though.

Please tell me if you think it's worth keeping both of these changes or only one of them or none - I'll amend as necessary and then get the PR merged finally.

hubertp

👍 re sbt change

…and numeric columns

Marking a bug in BoolStorage - TODO fix it

…9945

radeusgd self-assigned this Oct 11, 2022

radeusgd force-pushed the wip/radeusgd/filter-condition-is-in-183389945 branch from 06ecf9f to 0c10218 Compare October 12, 2022 10:33

radeusgd marked this pull request as ready for review October 12, 2022 12:52

radeusgd requested review from jdunkerley, 4e6, JaroslavTulach and hubertp as code owners October 12, 2022 12:52

radeusgd commented Oct 12, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Filter_Condition.enso Show resolved Hide resolved

radeusgd commented Oct 12, 2022

View reviewed changes

radeusgd force-pushed the wip/radeusgd/filter-condition-is-in-183389945 branch from f260e78 to c319934 Compare October 13, 2022 07:45

jdunkerley approved these changes Oct 13, 2022

View reviewed changes

hubertp approved these changes Oct 13, 2022

View reviewed changes

radeusgd force-pushed the wip/radeusgd/filter-condition-is-in-183389945 branch 2 times, most recently from 4c3bdee to 5eb9c3f Compare October 14, 2022 11:59

radeusgd added 15 commits October 14, 2022 17:43

Add Is_In and Not_In to base types

9c54cf1

checkpoint

e24b762

Initial fallback implementation, some basic tests

cea9198

checkpoint

727de68

Abstract-out numeric conversions, implement specializations for Text …

381a57e

…and numeric columns

Add more tests, implement other specializations

a5e988f

Implement vectorized is_in for booleans too

77826c5

Add tests for Integer-Decimal cross-equality and fix these

8e42f5e

Implement Database IN

96fcbe9

Fixes

d872af4

Formatting

e6f1e63

Actually measure efficiency of Table.filter Is_In

862bb2f

changelog

fe7e7fc

cr: modulus

0f9f1a6

cr: make is_in and like public - improving docs

ba2098b

radeusgd added 11 commits October 14, 2022 17:43

Add tests for Is_In on boolean columns

845fc58

Add a test checking the masking implementation of various columns

5295c81

Marking a bug in BoolStorage - TODO fix it

fix a bug in BoolStorage::applyMask

a81e0c5

Much more robust boolean tests, more efficient boolean operation

94e8c36

cr: rename internal ops to have underscores

66dfe91

obsolete TODO

41b262e

move tests around, new placement is more reasonable

6ac4e36

Generify SpecializedIsInOp

2e42d68

Simplify TypedStorage

e2b77c9

Make Storage aware of its (boxed) storage type

eb120cc

formatting

bc5b520

radeusgd force-pushed the wip/radeusgd/filter-condition-is-in-183389945 branch from ef188f2 to bc5b520 Compare October 14, 2022 15:43

jdunkerley approved these changes Oct 14, 2022

View reviewed changes

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Oct 15, 2022

mergify bot and others added 2 commits October 15, 2022 09:14

Merge branch 'develop' into wip/radeusgd/filter-condition-is-in-18338…

139366c

…9945

fixes after rebase

ae84e89

radeusgd force-pushed the wip/radeusgd/filter-condition-is-in-183389945 branch from 5913848 to ae84e89 Compare October 15, 2022 10:44

mergify bot merged commit 82de8f8 into develop Oct 15, 2022

mergify bot deleted the wip/radeusgd/filter-condition-is-in-183389945 branch October 15, 2022 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `Is_In` and `Not_In` to `Filter_Condition` #3790

Add support for `Is_In` and `Not_In` to `Filter_Condition` #3790

radeusgd commented Oct 11, 2022 •

edited

Loading

radeusgd Oct 12, 2022

hubertp Oct 13, 2022

JaroslavTulach Oct 13, 2022

radeusgd Oct 13, 2022

radeusgd Oct 12, 2022

jdunkerley left a comment

jdunkerley Oct 13, 2022

radeusgd Oct 14, 2022

hubertp left a comment

Add support for Is_In and Not_In to Filter_Condition #3790

Add support for Is_In and Not_In to Filter_Condition #3790

Conversation

radeusgd commented Oct 11, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

radeusgd Oct 12, 2022

Choose a reason for hiding this comment

hubertp Oct 13, 2022

Choose a reason for hiding this comment

JaroslavTulach Oct 13, 2022

Choose a reason for hiding this comment

radeusgd Oct 13, 2022

Choose a reason for hiding this comment

radeusgd Oct 12, 2022

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

jdunkerley Oct 13, 2022

Choose a reason for hiding this comment

radeusgd Oct 14, 2022

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

Add support for `Is_In` and `Not_In` to `Filter_Condition` #3790

Add support for `Is_In` and `Not_In` to `Filter_Condition` #3790

radeusgd commented Oct 11, 2022 •

edited

Loading