v0.37 pubsub/kvindexer: Handle big ints by jmalicevic · Pull Request #771 · cometbft/cometbft

jmalicevic · 2023-04-27T10:10:51Z

Closes #726 for 0.37.x.

This is a draft PR. The main goal is to make sure that big integers are parsed properly without a loss of precision. Int64 values will be processed as before.
It also fixes the old logic where ints and floats were treated the same if a float had a fraction due to the fact that the float was converted to an int before comparison.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

state/indexer/block/kv/kv_test.go

lasarojc · 2023-04-28T10:31:51Z

libs/pubsub/query/query.go

+				valueBig, ok := valueBig.SetString(number, 10)
+				if !ok {
+					err := fmt.Errorf(
+						"problem parsing %s as bigint (should never happen if the grammar is correct)",


"if the grammar is correct" is not very helpful to the user.
Could we replace it for something like "please report the error to the development team"?

As this is the default message in a bunch of other places and it actually makes sense - comparing the query to the query grammar, I would leave this?

libs/pubsub/query/query.go

libs/pubsub/query/query_test.go

sergio-mena

LGTM. I payed a lot of attention, but added some questions on parts I didn't fully understand.

state/indexer/block/kv/kv_test.go

sergio-mena · 2023-04-28T15:40:11Z

state/indexer/block/kv/kv_test.go

+			q:       query.MustParse("block.height < 2 AND block.height > 0 AND block.height > 0"),
+			results: []int64{1},
+		},
+		"query matches fields with big int and height - no match": {


Can we add a similar test case that is testing an off-by-1 bigint value? (testing precision not lost, as done further up in another file)

Added a use case, let me know if it covers what oyu had in mind.

state/indexer/block/kv/util.go

state/indexer/query_range.go

libs/pubsub/query/query.go

libs/pubsub/query/query_test.go

libs/pubsub/query/query.go

libs/pubsub/query/query_test.go

state/indexer/block/kv/kv.go

libs/pubsub/query/query.go

.changelog/unreleased/bug-fixes/771-kvindexer-parsing-big-ints.md

state/indexer/query_range.go

Co-authored-by: Lasaro <lasaro@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems>

sergio-mena

🥳

Replaced int64 with big.int Co-authored-by: Lasaro <lasaro@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems>

Replaced int64 with big.int Co-authored-by: Lasaro <lasaro@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Thane Thomson <connect@thanethomson.com>

rootulp · 2023-07-13T15:24:46Z

docs/app-dev/indexing-transactions.md

 occurred within the same event.

+# Event attribute value types
+Users can use anything as an event value. However, if the even attrbute value is a number, the following restrictions apply:


typo: even => event

Replaced int64 with big.int Co-authored-by: Lasaro <lasaro@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems>

First working version

609d239

jmalicevic added this to the 2023-Q2 milestone Apr 27, 2023

jmalicevic self-assigned this Apr 27, 2023

jmalicevic linked an issue Apr 27, 2023 that may be closed by this pull request

Using pubsub query with big values results in an int64 overflow #726

Closed

jmalicevic changed the base branch from main to v0.37.x April 27, 2023 10:11

jmalicevic changed the title ~~pubsub: Handle big ints~~ (WIP) pubsub: Handle big ints Apr 27, 2023

jmalicevic added 4 commits April 27, 2023 13:20

Removed needless complication with BigFloat

9088f89

Block indexer uses bigints now

7dc632f

First version of big int for txindex

441c0c4

Fixed pubsub tests failing after removing support for int64

dc39b49

jmalicevic commented Apr 27, 2023

View reviewed changes

state/indexer/block/kv/kv_test.go Show resolved Hide resolved

jmalicevic added 3 commits April 28, 2023 12:25

Refactored code, added test cases

3c7fef2

Merge branch 'v0.37.x' into jasmina/726-pubsub-int-overflow

564818f

Reverted error msg

1b6234a

jmalicevic marked this pull request as ready for review April 28, 2023 10:30

jmalicevic requested a review from a team as a code owner April 28, 2023 10:30

jmalicevic changed the title ~~(WIP) pubsub: Handle big ints~~ v0.37 pubsub: Handle big ints Apr 28, 2023

Removed nofrac

4f3886f

lasarojc reviewed Apr 28, 2023

View reviewed changes

sergio-mena approved these changes Apr 28, 2023

View reviewed changes

jmalicevic added 5 commits April 28, 2023 22:13

Addressed PR comments

39ea33e

removed int64 case statement

aadbbd4

Added tests for conditions

ff9f7d0

Added support for events whose value is float

ec61e80

Added new test case for lower rounding float

1dd27a7

sergio-mena reviewed May 1, 2023

View reviewed changes

state/indexer/block/kv/kv.go Outdated Show resolved Hide resolved

jmalicevic marked this pull request as draft May 1, 2023 21:42

jmalicevic added 3 commits May 2, 2023 10:23

Added proper big float parsing for pubsub and block indexer

b23dca4

Added parsing floats as int in the indexer -properly

6835c25

Resolved litner error

28c895b

jmalicevic added 5 commits May 2, 2023 14:04

Added changelog`

07bd8b8

Added documentation

1927758

Added api breaking changes

6aa23cf

fixed doc

28d63b0

Applied pr review comment;

f2bae77

jmalicevic marked this pull request as ready for review May 2, 2023 13:19

sergio-mena reviewed May 2, 2023

View reviewed changes

state/indexer/block/kv/kv.go Outdated Show resolved Hide resolved

sergio-mena reviewed May 2, 2023

View reviewed changes

libs/pubsub/query/query.go Show resolved Hide resolved

lasarojc reviewed May 2, 2023

View reviewed changes

.changelog/unreleased/bug-fixes/771-kvindexer-parsing-big-ints.md Outdated Show resolved Hide resolved

lasarojc approved these changes May 2, 2023

View reviewed changes

sergio-mena reviewed May 2, 2023

View reviewed changes

state/indexer/query_range.go Show resolved Hide resolved

jmalicevic and others added 3 commits May 2, 2023 16:43

Apply suggestions from code review

be77652

Co-authored-by: Lasaro <lasaro@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems>

Reverted breaking change, removed support for reading floats from db

1875e94

Updated changelog

2c599de

sergio-mena approved these changes May 2, 2023

View reviewed changes

jmalicevic added 3 commits May 3, 2023 10:00

fixed linter error

48e77c3

applied pr comment

e437d4a

Fixed linter

0ff73c5

jmalicevic added the backport-to-v0.34.x label May 3, 2023

jmalicevic merged commit 9267594 into v0.37.x May 3, 2023

jmalicevic deleted the jasmina/726-pubsub-int-overflow branch May 3, 2023 08:38

jmalicevic changed the title ~~v0.37 pubsub: Handle big ints~~ v0.37 pubsub/kvindexer: Handle big ints May 5, 2023

jmalicevic added a commit that referenced this pull request May 5, 2023

v0.37 pubsub: Handle big ints (#771)

44c0c40

Replaced int64 with big.int Co-authored-by: Lasaro <lasaro@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems>

jmalicevic mentioned this pull request May 5, 2023

Backport of PR #771 to 0.34 : big int parsing #798

Merged

jmalicevic mentioned this pull request May 11, 2023

Using pubsub query with big values results in an int64 overflow #726

Closed

rootulp mentioned this pull request Jul 13, 2023

Txs endpoint returns error on range operators celestiaorg/celestia-app#2098

Closed

4 tasks

rootulp reviewed Jul 13, 2023

View reviewed changes

faddat mentioned this pull request Jan 5, 2024

faddat/reversions for fixed length ComposableFi/cometbft#2

Closed

3 tasks

DongLieu pushed a commit to DongCoNY/cometbft that referenced this pull request May 13, 2024

v0.37 pubsub: Handle big ints (cometbft#771)

b852e9e

Replaced int64 with big.int Co-authored-by: Lasaro <lasaro@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems>

Conversation

jmalicevic commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Uh oh!

lasarojc Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

jmalicevic May 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sergio-mena Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

jmalicevic Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

rootulp Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmalicevic commented Apr 27, 2023 •

edited

Loading