Skip to content
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

crud: add readview support #372

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Conversation

better0fdead
Copy link
Contributor

@better0fdead better0fdead commented Sep 14, 2023

Added readview support for select and pairs.
e.g

tarantool> a = crud.readview('1')

tarantool> crud.select('customers', nil, {fullscan=true})
---
- metadata: [{'name': 'id', 'type': 'unsigned'}, {'name': 'bucket_id', 'type': 'unsigned'},
    {'name': 'name', 'type': 'string'}, {'name': 'age', 'type': 'number'}]
  rows:
  - [1, 477, 'Elizabeth', 12]
  - [2, 401, 'Mary', 46]
  - [3, 2804, 'David', 33]
  - [4, 1161, 'William', 81]
  - [5, 1172, 'Jack', 35]
  - [6, 1064, 'William', 25]
  - [7, 693, 'Elizabeth', 18]
- null
...

tarantool> a:select('customers', nil, {batch_size=1,fullscan=true})
---
- metadata: [{'name': 'id', 'type': 'unsigned'}, {'name': 'bucket_id', 'type': 'unsigned'},
    {'name': 'name', 'type': 'string'}, {'name': 'age', 'type': 'number'}]
  rows:
  - [1, 477, 'Elizabeth', 12]
  - [2, 401, 'Mary', 46]
  - [3, 2804, 'David', 33]
  - [4, 1161, 'William', 81]
  - [5, 1172, 'Jack', 35]
  - [6, 1064, 'William', 25]
  - [7, 693, 'Elizabeth', 18]
- null
...

tarantool> crud.insert('customers', {8, box.NULL, 'Elizabeth', 23})
---
- rows:
  - [8, 185, 'Elizabeth', 23]
  metadata: [{'name': 'id', 'type': 'unsigned'}, {'name': 'bucket_id', 'type': 'unsigned'},
    {'name': 'name', 'type': 'string'}, {'name': 'age', 'type': 'number'}]
- null
...

tarantool> a:select('customers', nil, {batch_size=1,fullscan=true})
---
- metadata: [{'name': 'id', 'type': 'unsigned'}, {'name': 'bucket_id', 'type': 'unsigned'},
    {'name': 'name', 'type': 'string'}, {'name': 'age', 'type': 'number'}]
  rows:
  - [1, 477, 'Elizabeth', 12]
  - [2, 401, 'Mary', 46]
  - [3, 2804, 'David', 33]
  - [4, 1161, 'William', 81]
  - [5, 1172, 'Jack', 35]
  - [6, 1064, 'William', 25]
  - [7, 693, 'Elizabeth', 18]
- null
...

tarantool> crud.select('customers', nil, {fullscan=true})
---
- metadata: [{'name': 'id', 'type': 'unsigned'}, {'name': 'bucket_id', 'type': 'unsigned'},
    {'name': 'name', 'type': 'string'}, {'name': 'age', 'type': 'number'}]
  rows:
  - [1, 477, 'Elizabeth', 12]
  - [2, 401, 'Mary', 46]
  - [3, 2804, 'David', 33]
  - [4, 1161, 'William', 81]
  - [5, 1172, 'Jack', 35]
  - [6, 1064, 'William', 25]
  - [7, 693, 'Elizabeth', 18]
  - [8, 185, 'Elizabeth', 23]
- null
...
tarantool> tuples = {}
---
...

tarantool> for _, tuple in a:pairs('customers', {{'<=', 'age', 35}},{use_tomap=false})  do
table.insert(tuples, tuple)
end
---
...

tarantool> tuples
---
- - [5, 1172, 'Jack', 35]
  - [3, 2804, 'David', 33]
  - [6, 1064, 'William', 25]
  - [7, 693, 'Elizabeth', 18]
  - [1, 477, 'Elizabeth', 12]
...

Closes #343

I didn't forget about

  • [ x] Tests
  • [ x] Changelog
  • [ x] Documentation

Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Thank you for your patch! That's a rather nice implementation, but we may improve it in some ways. A couple of things definitely need to be changed, some of them may be ignored for now.

crud.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
crud/readview.lua Show resolved Hide resolved
crud/compare/plan.lua Outdated Show resolved Hide resolved
crud/select/executor.lua Outdated Show resolved Hide resolved
crud/select/merger.lua Outdated Show resolved Hide resolved
crud/compare/filters.lua Outdated Show resolved Hide resolved
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Thank you for the updates. Most of the crucial things seems fine, there are some minor things that should be fixed before merge. And we still lack some tests. You may start to write README API description after adding missing options to .new and :close

crud.lua Outdated Show resolved Hide resolved
crud/readview.lua Outdated Show resolved Hide resolved
crud/readview.lua Outdated Show resolved Hide resolved
crud/readview.lua Outdated Show resolved Hide resolved
crud/readview.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Show resolved Hide resolved
test/integration/pairs_readview_test.lua Outdated Show resolved Hide resolved
crud/readview.lua Outdated Show resolved Hide resolved
@better0fdead better0fdead force-pushed the better0fdead/readview branch 3 times, most recently from 33f36c7 to 73fe4a9 Compare September 26, 2023 02:27
@better0fdead better0fdead marked this pull request as ready for review September 26, 2023 02:27
@better0fdead better0fdead force-pushed the better0fdead/readview branch 5 times, most recently from 0b5bea5 to b2f352c Compare September 26, 2023 05:25
@better0fdead better0fdead force-pushed the better0fdead/readview branch 2 times, most recently from ff08beb to 19b377c Compare September 26, 2023 10:52
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! Seems awesome, I think I'll merge this one after you resolve remaining comments.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Outdated Show resolved Hide resolved
test/integration/select_readview_test.lua Show resolved Hide resolved
@better0fdead better0fdead force-pushed the better0fdead/readview branch 2 times, most recently from b8b31e0 to af8bcfa Compare September 26, 2023 19:20
Added read view support for select and pairs.

Closes #343
@DifferentialOrange DifferentialOrange merged commit 1afe790 into master Sep 27, 2023
27 checks passed
@DifferentialOrange DifferentialOrange deleted the better0fdead/readview branch September 27, 2023 07:01
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.

Support readview
2 participants