Skip to content

Conversation

MeirShpilraien
Copy link

@MeirShpilraien MeirShpilraien commented Mar 20, 2021

  • cherry pick to 2.0

filipecosta90
filipecosta90 previously approved these changes Mar 20, 2021
@MeirShpilraien
Copy link
Author

Tests will pass when this will be merged:
RedisLabsModules/RLTest#104

rafie
rafie previously approved these changes Mar 21, 2021
Copy link
Contributor

@ashtul ashtul left a comment

Choose a reason for hiding this comment

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

These tests are for the payload.

@@ -207,10 +207,10 @@ def testPayload(env):
for _ in env.retry_with_rdb_reload():
waitForIndex(env, 'things')
res = env.cmd('ft.search', 'things', 'foo')
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add 'WITHPAYLOAD`

Copy link
Author

@MeirShpilraien MeirShpilraien Mar 21, 2021

Choose a reason for hiding this comment

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

There is withpayloads right below..

@@ -207,10 +207,10 @@ def testPayload(env):
for _ in env.retry_with_rdb_reload():
waitForIndex(env, 'things')
res = env.cmd('ft.search', 'things', 'foo')
env.assertEqual(toSortedFlatList(res), toSortedFlatList([1L, 'thing:foo', ['name', 'foo', 'payload', 'stuff']]))
env.assertEqual(toSortedFlatList(res), toSortedFlatList([1L, 'thing:foo', ['name', 'foo']]))

res = env.cmd('ft.search', 'things', 'foo', 'withpayloads')
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Author

Choose a reason for hiding this comment

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

This is withpayloads

@@ -223,10 +223,10 @@ def testBinaryPayload(env):
for _ in env.retry_with_rdb_reload():
waitForIndex(env, 'things')
res = env.cmd('ft.search', 'things', 'foo')
env.assertEqual(toSortedFlatList(res), toSortedFlatList([1L, 'thing:foo', ['name', 'foo', 'payload', '\x00\xAB\x20']]))
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Author

Choose a reason for hiding this comment

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

same


res = env.cmd('ft.search', 'things', 'foo', 'withpayloads')
env.assertEqual(toSortedFlatList(res), toSortedFlatList([1L, 'thing:foo', '\x00\xAB\x20', ['name', 'foo', 'payload', '\x00\xAB\x20']]))
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Author

Choose a reason for hiding this comment

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

same

ashtul
ashtul previously approved these changes Mar 21, 2021
@MeirShpilraien MeirShpilraien dismissed stale reviews from ashtul and rafie via 4988a18 March 22, 2021 12:57
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #1914 (409ca43) into master (56ae587) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1914   +/-   ##
=======================================
  Coverage   80.30%   80.30%           
=======================================
  Files         148      148           
  Lines       21503    21504    +1     
=======================================
+ Hits        17268    17269    +1     
  Misses       4235     4235           
Impacted Files Coverage Δ
src/rlookup.c 78.94% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56ae587...409ca43. Read the comment docs.

@MeirShpilraien
Copy link
Author

@rafie @ashtul can we merged this?

@ashtul
Copy link
Contributor

ashtul commented Mar 24, 2021

LGTM 👍🏻

@MeirShpilraien MeirShpilraien merged commit 241528c into master Mar 24, 2021
@MeirShpilraien MeirShpilraien deleted the do_not_return_payload_as_field branch March 24, 2021 10:53
MeirShpilraien pushed a commit that referenced this pull request Mar 24, 2021
* do not return payload as field

* fix tests

* fix tests

* fix tests

* fix tests

Co-authored-by: Rafi Einstein <[email protected]>
MeirShpilraien pushed a commit that referenced this pull request Mar 29, 2021
* do not return payload as field

* fix tests

* fix tests

* fix tests

* fix tests

Co-authored-by: Rafi Einstein <[email protected]>

Co-authored-by: Rafi Einstein <[email protected]>
@ashtul ashtul mentioned this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants