neo4jrb / neo4j-core

A simple unified API that can access both the server and embedded Neo4j database. Used by the neo4j gem
MIT License
99 stars 80 forks source link

Add support for querying non-integer ranges #295

Closed jgaskins closed 7 years ago

jgaskins commented 7 years ago

Passing a time or date range wasn't working when serialized as non-integer values (I've been using strings for query readability) for two reasons:

This commit expands support for ranges to include non-numeric ranges and non-integer exclusive ranges through the use of Range#end rather than Range#max. It leaves integer-range support alone in case Neo4j has internal optimizations for it. I also included a spec for the float-range exclusive case and confirmed it raised a TypeError before making any changes to the lib code.

Pings: @cheerfulstoic (seems to have the most commits on this file)

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.01%) to 92.384% when pulling 61ec6b5ae0e98bea3fa246dbb52735b81523c7ff on jgaskins:non-integer-ranges into 69f54584c68c7a5f6879f0fb6174fb3257c993ce on neo4jrb:master.

cheerfulstoic commented 7 years ago

This is awesome, thanks!

It looks like CI is failing because of rubocop. Could you run rubocop -aD locally and make sure that it passes?

jgaskins commented 7 years ago

UUGGGGHH this pushes it just past the threshold of so many things Rubocop tracks! :-)

Lemme see what I can do here.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.05%) to 92.322% when pulling 7e8320eada03beffc6ed511fb3b33279337cc6a0 on jgaskins:non-integer-ranges into 69f54584c68c7a5f6879f0fb6174fb3257c993ce on neo4jrb:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.01%) to 92.386% when pulling 916d1b577f75689c03c9c659c6f18d5921f8563b on jgaskins:non-integer-ranges into 69f54584c68c7a5f6879f0fb6174fb3257c993ce on neo4jrb:master.

cheerfulstoic commented 7 years ago

The Metrics/ClassLength is a larger issue which I can probably attack if it's taking too much time ;)

jgaskins commented 7 years ago

@cheerfulstoic haha I was just taking another look at this. I couldn't figure out a way to make the class smaller without more refactoring than I'm comfortable doing here and was about to ask if we should just bump the threshold up a notch. :-)

jgaskins commented 7 years ago

Regarding the Style/AlignParameters cop, I figured I could either inline the whole thing (makes it look more like the other examples but hard to see the params at a glance) or pull one of these maneuvers:

    describe '.where(q: { created_at: Date.new(2017, 6, 1)..Date.new(2017, 6, 3) })' do
      it_generates(
        'WHERE (q.created_at >= {q_created_at_range_min} AND q.created_at <= {q_created_at_range_max})',
        q_created_at_range_min: Date.new(2017, 6, 1),
        q_created_at_range_max: Date.new(2017, 6, 3)
      )

… which makes it a bit easier to read but then it's nothing like the rest.

cheerfulstoic commented 7 years ago

Yeah, the QueryClauses module is a beast ;) . I'll merge and have a look at it now

cheerfulstoic commented 7 years ago

Working on this and I just did 10 trials each comparing IN RANGE() to x <= y AND y <= z and found that the latter was much faster, so that should simplify the code (I'm doing that now):

WITH RANGE(1, 5000) AS list
UNWIND list as i
RETURN i IN RANGE(1500, 3500)

1375 ms
1018 ms
1002 ms
970 ms
1047 ms
1090 ms
997 ms
990 ms
953 ms
1030 ms

WITH RANGE(1, 5000) AS list
UNWIND list as i
RETURN 1500 <= i AND i <= 3500

73 ms
89 ms
64 ms
59 ms
78 ms
47 ms
78 ms
65 ms
76 ms
83 ms
cheerfulstoic commented 7 years ago

Just released 7.2.0 of the neo4j-core gem with this addition. Thanks again!

jgaskins commented 7 years ago

Awesome! 👍 Glad I could help!