Skip to content

Adds support for BigFloat #3981

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

Merged
merged 2 commits into from
Aug 20, 2024
Merged

Adds support for BigFloat #3981

merged 2 commits into from
Aug 20, 2024

Conversation

harshil-goel
Copy link
Contributor

@harshil-goel harshil-goel commented Sep 13, 2019

This PR adds BigFloat as another scalar type in schema.
For now, it supports storing and retrieving 200 precision float, which isn't possible with just a float64.

Work Remaining:

  • Precision is hardcoded to 200
  • No support for aggregate functions
  • Add tests

This change is Reviewable

@harshil-goel harshil-goel requested a review from a team September 13, 2019 12:32
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@harshil-goel you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Although subsequent PR may change the value of 200 It may be valuable to abstracting the value in a variable. Future iterations can perhaps change the value of 200 to any or other integer, but it also opens up the possibility of the precision value being passed around and extended by callers who might do nothing, update the variable or modify if using a function.


Reviewed with ❤️ by PullRequest

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

I'll be having another look at this tomorrow. For now please add some more test cases in query tests.

Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @harshil-goel, @manishrjain, and @pullrequest[bot])


dgraph/cmd/alpha/upsert_test.go, line 1595 at r1 (raw file):

}

func TestBigFloat(t *testing.T) {

This doesn't seem the right place to add these tests. Please add more tests storing and retrieving various 200 precision float numbers.


protos/pb.proto, line 264 at r1 (raw file):

		PASSWORD = 8;
		STRING = 9;
    OBJECT = 10;

Could you please fix the indentation for OBJECT?


tok/tok.go, line 181 at r1 (raw file):

func (t BigFloatTokenizer) Type() string { return "bigfloat" }
func (t BigFloatTokenizer) Tokens(v interface{}) ([]string, error) {
	return []string{(v.(*big.Float)).String()}, nil

Doc for this says

String formats x like x.Text('g', 10) i.e. it only stores with precision 10. I doubt this will work for filtering. We should remove the addition of this tokenizer in this PR and add it back later when we support different types of queries for it.


types/conversion.go, line 71 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Can the precision value (200) be saved as a constant? It is used in multiple places and if in the future this becomes configurable it will be easier to maintain as a single sentinel or injected value that all callsites read from.

Yes I agree. Should be a constant.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

1 Message
⚠️ Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers.

Copy link
Contributor Author

@harshil-goel harshil-goel left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 17 files reviewed, 14 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @manishrjain, @pawanrawal, and @pullrequest[bot])


dgraph/cmd/alpha/upsert_test.go, line 1595 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This doesn't seem the right place to add these tests. Please add more tests storing and retrieving various 200 precision float numbers.

Done.


protos/pb.proto, line 264 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Could you please fix the indentation for OBJECT?

Done.


tok/tok.go, line 53 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Have your editor align with the other values or please do so manually.

Done.


tok/tok.go, line 181 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Doc for this says

String formats x like x.Text('g', 10) i.e. it only stores with precision 10. I doubt this will work for filtering. We should remove the addition of this tokenizer in this PR and add it back later when we support different types of queries for it.

Done.


tok/tok.go, line 27 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

correct import.

Done.


types/conversion.go, line 71 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Yes I agree. Should be a constant.

Done.


types/conversion.go, line 33 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

run go import

Done.


types/conversion.go, line 71 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

change name val to something else, as val is a type

Done.


types/conversion.go, line 126 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

same

Done.


types/conversion.go, line 180 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

same

Done.


types/scalar_types.go, line 173 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Another case of precision being a magic number. If this changes in one place it should be done in all.

Done.


types/scalar_types.go, line 24 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

correct import

Done.


worker/export.go, line 5 at r3 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 108 characters (from lll)

Done.


worker/export.go, line 87 at r3 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

	types.BigFloatID: "big:bigfloat",

Done.

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 12 files at r1, 2 of 5 files at r2, 8 of 9 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @harshil-goel, @manishrjain, @pawanrawal, and @pullrequest[bot])


dgraph/cmd/alpha/upsert_test.go, line 1595 at r1 (raw file):

Previously, harshil-goel wrote…

Done.

Is this test actually using upsert? There are mutations test already. Somewhere like in mutation_test.go. That might be a better place for this tests if this test is not using the upsert functionality.


query/aggregator.go, line 145 at r4 (raw file):

				return errors.Errorf("Wrong type encountered for func %q", ag.name)
			}
			v.Value = math.Log(l)

Why are logs and exps not supported for big floats?


query/aggregator.go, line 160 at r4 (raw file):

				v.Value = negResult
			} else {
				if !isIntOrFloat {

To make the code easier to read a little bit, I would add the type check at the beginning of each case. So something like:

case "u-":
  if !isBigFloat && !isIntOrFloat {
    return errors.Errorf("Wrong type encountered for func %q", ag.name)
  }

then you can get rid of the if inside the else below. Same thing for the rest of the cases.


query/aggregator.go, line 242 at r4 (raw file):

			va.Value = value
		} else {
			if !isIntOrFloat {

same thing about checking for a valid type first like in the unary operators.


types/conversion.go, line 227 at r4 (raw file):

				return to, cantConvert(fromID, toID)
			}

nit: extra line not needed here


types/conversion.go, line 565 at r4 (raw file):

		}
		return &api.Value{Val: &api.Value_BytesVal{BytesVal: v}}, nil
	// Geo, datetime and BigFloat are stored in binary format in the N-Quad, so lets

nit: "datetime, and BigFloat"


types/scalar_types.go, line 57 at r4 (raw file):

	// UndefinedID represents the undefined type.
	UndefinedID = TypeID(100)
	// BigFloatID represents the arbitrary precision type

nit: period at the end of comments.

Copy link
Contributor Author

@harshil-goel harshil-goel left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @manishrjain, @martinmr, @pawanrawal, and @pullrequest[bot])


dgraph/cmd/alpha/upsert_test.go, line 1595 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Is this test actually using upsert? There are mutations test already. Somewhere like in mutation_test.go. That might be a better place for this tests if this test is not using the upsert functionality.

I have written upsert tests, as they would be basically integration tests in which we can test querying, aggregating and storing big float. I will add more tests just for mutation


query/aggregator.go, line 145 at r4 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Why are logs and exps not supported for big floats?

Big.Float doesn't support pow or log. They had a proposal for it, but nothing really happened.
"golang/go#14102"
Maybe we can, later on, support our own implementation using Taylor series.


query/aggregator.go, line 160 at r4 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

To make the code easier to read a little bit, I would add the type check at the beginning of each case. So something like:

case "u-":
  if !isBigFloat && !isIntOrFloat {
    return errors.Errorf("Wrong type encountered for func %q", ag.name)
  }

then you can get rid of the if inside the else below. Same thing for the rest of the cases.

Done.


query/aggregator.go, line 242 at r4 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

same thing about checking for a valid type first like in the unary operators.

Done.


types/conversion.go, line 227 at r4 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

nit: extra line not needed here

Done.


types/conversion.go, line 565 at r4 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

nit: "datetime, and BigFloat"

Done.


types/scalar_types.go, line 57 at r4 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

nit: period at the end of comments.

Done

@harshil-goel harshil-goel force-pushed the harshil-goel/big-int-support branch from addd66e to c1ce90d Compare September 18, 2019 09:34
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

⚠️ Warning

PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More

@harshil-goel harshil-goel changed the title [WIP] Adds support for BigFloat Adds support for BigFloat Sep 18, 2019
Copy link
Member

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 17 files reviewed, 33 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @harshil-goel, @manishrjain, @martinmr, @pawanrawal, and @pullrequest[bot])


chunker/rdf_parser.go, line 333 at r5 (raw file):

	"xs:float":           types.FloatID,
	"xs:base64Binary":    types.BinaryID,
	"big:bigfloat":       types.BigFloatID,

Why is it big?


query/aggregator.go, line 122 at r5 (raw file):

	}

	var isIntOrFloat bool

Does it make sense to use an enum here instead of two variables? You can define INT_OR_FLOAT as one of the enum value.


query/aggregator.go, line 160 at r5 (raw file):

			if isBigFloat {
				value := v.Value.(big.Float)
				negResult := *new(big.Float).SetPrec(types.BigFloatPrecision)

can just call it res


query/aggregator.go, line 175 at r5 (raw file):

			if isBigFloat {
				value := v.Value.(big.Float)
				sqrtResult := *new(big.Float).SetPrec(types.BigFloatPrecision)

The *new() syntax does now seem idiomatic Go.


query/aggregator.go, line 175 at r5 (raw file):

			if isBigFloat {
				value := v.Value.(big.Float)
				sqrtResult := *new(big.Float).SetPrec(types.BigFloatPrecision)

call it just res


query/aggregator.go, line 230 at r5 (raw file):

	va := ag.result
	var prevValue big.Float

what is prevValue?


query/aggregator.go, line 289 at r5 (raw file):

		if isBigFloat {
			if prevValue.Cmp(new(big.Float)) == 0 {

define a constant for zero value


query/aggregator.go, line 290 at r5 (raw file):

		if isBigFloat {
			if prevValue.Cmp(new(big.Float)) == 0 {
				return errors.Errorf("Division by zero")

define this error at the top and reuse it below


query/aggregator.go, line 426 at r5 (raw file):

	} else if ag.result.Tid == types.FloatID {
		v = ag.result.Value.(float64)
	} else if ag.result.Tid == types.BigFloatID {

Fix this logic, you are only returning from this if case[ It will be hard to follow later.


query/math_test.go, line 120 at r5 (raw file):

		{in: &mathTree{
			Fn: "+",
			Child: []*mathTree{

Add some tests for float values


query/math_test.go, line 333 at r5 (raw file):

func TestBigFloatMathsBoolean(t *testing.T) {
	tests := []struct {

Just see integer mostly in the tests, use some float tests too.


query/outputnode.go, line 172 at r5 (raw file):

		return geojson.Marshal(v.Value.(geom.T))
	case types.BigFloatID:
		var b big.Float

declaration is not needed,


query/query4_test.go, line 21 at r5 (raw file):

import (
	"context"
	// "encoding/json"

remove extra commented imports


query/query4_test.go, line 615 at r5 (raw file):

			uid
		}
	}`

Add error test cases for providing string value, wrong value with two decimals


types/conversion.go, line 209 at r5 (raw file):

				b, err := t.MarshalText()
				if err != nil {
					return to, errors.Errorf("Error while parsing %s", err.Error())

Error while conversion*


types/conversion.go, line 292 at r5 (raw file):

				}
			case BigFloatID:
				*res = new(big.Float).SetPrec(BigFloatPrecision).SetInt64(0)

Use if/else instead, do not create two objects

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r5.
Reviewable status: all files reviewed, 30 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])


query/aggregator.go, line 175 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

The *new() syntax does now seem idiomatic Go.

yes, remove this and just change v.Value = sqrtResult to v.Value = &sqrtResult


types/conversion.go, line 209 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Error while conversion*

"while converting" sounds slightly better

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 30 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @pawanrawal, and @pullrequest[bot])


dgraph/cmd/alpha/upsert_test.go, line 1595 at r1 (raw file):

Previously, harshil-goel wrote…

I have written upsert tests, as they would be basically integration tests in which we can test querying, aggregating and storing big float. I will add more tests just for mutation

sounds good.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

:lgtm:

Had a look at the PR, it looks good. Good work @harshil-goel. I just have comments about some cosmetic changes about naming and a bit of refactoring. Discuss it with @mangalaman93 and make the changes that seem appropriate. A couple of other things that you should verify before merging this.

  1. Verify that exporting data and schema with this new type works. We should have a test that you should be able to modify to test for this type as well. Also you should be able to re-import it.
  2. See that a user can set a predicate with the schema as bigfloat and the type is returned as part of the schema query.
  3. Our clients (Go/Java etc.) also have to support setting this data.
  4. Docs need to be updated.

Reviewable status: all files reviewed, 33 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, and @pullrequest[bot])


dgraph/cmd/alpha/upsert_test.go, line 1623 at r5 (raw file):

	require.NoError(t, err)

	expectedRes := `{"data":{"q":[{"name":"user1","amount":10.00000000000000000001}]}}`

I am assuming this test would fail if the schema for amount wasfloat instead of bigfloat. If not then we should change the value so that it does.


query/aggregator.go, line 123 at r5 (raw file):

	var isIntOrFloat bool
	var isBigFloat bool

So I see this variable being set to true initially when v the incoming value is of type BigFloatID and later when va the aggregated result value is of type BigFloatID. It shouldn't have dual intentions as it would lead to errors I believe. Please add a comment about what is this used for and have a single purpose. Same for isIntOrFloat. If you decide to use an enum as Aman says, then you could have two of them one for v and other for va later.


query/aggregator.go, line 139 at r5 (raw file):

	var res types.Val
	if isUnary(ag.name) {

Should we break this block into its own function to make it clear that this is only for handling unary aggregations. This block is now 100 lines long and we return at the end of it.


query/aggregator.go, line 154 at r5 (raw file):

			res = v
		case "u-":
			if !isBigFloat && !isIntOrFloat {

What does the u- function denote? Add a comment please.


query/aggregator.go, line 179 at r5 (raw file):

				v.Value = sqrtResult
			} else {
				v.Value = math.Sqrt(l)

Can we rename l to something more descriptive?


query/aggregator.go, line 230 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

what is prevValue?

This needs more comments or a better name, I had to read a lot of code to understand what it was. It seems to be the bigFloat representation of the value that we wish to add to the aggregated value.


types/sort.go, line 207 at r5 (raw file):

		return aOk && bOk && aVal == bVal
	case BigFloatID:
		aVal, aOk := a.Value.(big.Float)

You don't need to check ok here as at this point if the value is not a big.Float then there is some error in our code and its better to let it panic then to silently ignore the error. Notice how less also doesn't do it above.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

I also ran go test -coverprofile=coverage.out and go tool cover -html=coverage.out in the query package to see the test coverage. Have a look at that and please add test cases for the new code that has been added. From the result, I can notice that test case for % with bigfloat is missing.

Reviewable status: all files reviewed, 33 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, and @pullrequest[bot])

if va.Tid != types.IntID && va.Tid != types.FloatID {
isIntOrFloat = false
}
func (ag *aggregator) ApplyBinaryFunction(v, va types.Val, vBase argType, vFloat float64, vBigFloat big.Float) error {

Choose a reason for hiding this comment

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

line is 118 characters (from lll)

Copy link
Contributor Author

@harshil-goel harshil-goel left a comment

Choose a reason for hiding this comment

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

Actually just like pow and log, & is not supported for big Float. I must have written that code by mistake. I have removed it now.
I will make a different PR for clients and docs

Reviewable status: 9 of 18 files reviewed, 34 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @mangalaman93, @manishrjain, @martinmr, @pawanrawal, and @pullrequest[bot])


chunker/rdf_parser.go, line 333 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Why is it big?

Done.


dgraph/cmd/alpha/upsert_test.go, line 1623 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I am assuming this test would fail if the schema for amount wasfloat instead of bigfloat. If not then we should change the value so that it does.

Yeah, it fails for float.


query/aggregator.go, line 122 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Does it make sense to use an enum here instead of two variables? You can define INT_OR_FLOAT as one of the enum value.

Done.


query/aggregator.go, line 123 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

So I see this variable being set to true initially when v the incoming value is of type BigFloatID and later when va the aggregated result value is of type BigFloatID. It shouldn't have dual intentions as it would lead to errors I believe. Please add a comment about what is this used for and have a single purpose. Same for isIntOrFloat. If you decide to use an enum as Aman says, then you could have two of them one for v and other for va later.

Done.


query/aggregator.go, line 139 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should we break this block into its own function to make it clear that this is only for handling unary aggregations. This block is now 100 lines long and we return at the end of it.

Done.


query/aggregator.go, line 154 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What does the u- function denote? Add a comment please.

Done.


query/aggregator.go, line 160 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

can just call it res

Done.


query/aggregator.go, line 175 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

yes, remove this and just change v.Value = sqrtResult to v.Value = &sqrtResult

Done.


query/aggregator.go, line 175 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

call it just res

Done.


query/aggregator.go, line 179 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can we rename l to something more descriptive?

Done.


query/aggregator.go, line 230 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This needs more comments or a better name, I had to read a lot of code to understand what it was. It seems to be the bigFloat representation of the value that we wish to add to the aggregated value.

Done.


query/aggregator.go, line 289 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

define a constant for zero value

Done.


query/aggregator.go, line 290 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

define this error at the top and reuse it below

Done.


query/aggregator.go, line 426 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Fix this logic, you are only returning from this if case[ It will be hard to follow later.

Done.


query/aggregator.go, line 210 at r6 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 118 characters (from lll)

Done.


query/math_test.go, line 120 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Add some tests for float values

Done.


query/math_test.go, line 333 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Just see integer mostly in the tests, use some float tests too.

Done.


query/outputnode.go, line 172 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

declaration is not needed,

Done.


query/query4_test.go, line 21 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

remove extra commented imports

Done.


query/query4_test.go, line 615 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Add error test cases for providing string value, wrong value with two decimals

Done.


types/conversion.go, line 209 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

"while converting" sounds slightly better

Done.


types/conversion.go, line 292 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Use if/else instead, do not create two objects

Done.h


types/sort.go, line 207 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

You don't need to check ok here as at this point if the value is not a big.Float then there is some error in our code and its better to let it panic then to silently ignore the error. Notice how less also doesn't do it above.

Done.

Copy link
Member

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 18 files reviewed, 25 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, @martinmr, @pawanrawal, and @pullrequest[bot])


query/aggregator.go, line 290 at r5 (raw file):

Previously, harshil-goel (Harshil Goel) wrote…

Done.

Don't see it Done


query/aggregator.go, line 114 at r7 (raw file):

}

func (ag *aggregator) ApplyVal(v types.Val) error {

Have you reordered function definitions? Very difficult to review it now.


query/aggregator.go, line 115 at r7 (raw file):

}

type argType int

You could instead call it numType


types/conversion.go, line 292 at r5 (raw file):

Previously, harshil-goel (Harshil Goel) wrote…

Done.h

Not Done either

@mangalaman93 mangalaman93 self-requested a review September 23, 2019 09:42
@harshil-goel harshil-goel force-pushed the harshil-goel/big-int-support branch from 1414468 to a073ed2 Compare September 23, 2019 10:26
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Looks better. I am assuming you have checked that exporting the data using /admin/export and reimporting using live loader works fine?

Reviewed 3 of 12 files at r1, 2 of 9 files at r3, 1 of 2 files at r4, 3 of 6 files at r5, 4 of 9 files at r6, 5 of 5 files at r8.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @harshil-goel, @mangalaman93, @manishrjain, and @pullrequest[bot])


query/aggregator.go, line 179 at r5 (raw file):

Previously, harshil-goel (Harshil Goel) wrote…

Done.

Its still called l.


query/aggregator.go, line 130 at r8 (raw file):

	}

	var vBase numType

Should this be valType as float, bigfloat etc aren't really Base. Those are decimal, binary etc.


query/aggregator.go, line 131 at r8 (raw file):

	var vBase numType
	var vFloat float64

valFloat


query/aggregator.go, line 155 at r8 (raw file):

	va := ag.result
	var vBigFloat big.Float

valBigFLoat


query/aggregator.go, line 156 at r8 (raw file):

	va := ag.result
	var vBigFloat big.Float
	var vaBase numType

vaType

@MichelDiz MichelDiz added the dgraph Issue or PR created by an internal Dgraph contributor. label Mar 7, 2023
Copy link

github-actions bot commented Aug 7, 2024

This PR has been stale for 60 days and will be closed automatically in 7 days. Comment to keep it open.

@github-actions github-actions bot added the Stale label Aug 7, 2024
@shivaji-kharse shivaji-kharse force-pushed the harshil-goel/big-int-support branch from f96d297 to 3251fc9 Compare August 8, 2024 06:49
@shivaji-kharse shivaji-kharse requested a review from a team August 8, 2024 06:49
@shivaji-kharse shivaji-kharse force-pushed the harshil-goel/big-int-support branch from 3251fc9 to 25a5697 Compare August 8, 2024 09:55
@github-actions github-actions bot removed the Stale label Aug 9, 2024
@shivaji-kharse shivaji-kharse force-pushed the harshil-goel/big-int-support branch 5 times, most recently from d834f58 to 7ad53bc Compare August 13, 2024 09:13
Copy link
Member

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, just one comment.!

@@ -255,6 +274,42 @@ func Convert(from Val, toID TypeID) (Val, error) {
return to, cantConvert(fromID, toID)
}
}
case BigFloatID:
{
Copy link
Member

Choose a reason for hiding this comment

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

why is this in a block?

@mangalaman93 mangalaman93 force-pushed the harshil-goel/big-int-support branch from 7ad53bc to 4e7bada Compare August 19, 2024 14:59
@mangalaman93 mangalaman93 enabled auto-merge (squash) August 20, 2024 09:51
@mangalaman93 mangalaman93 merged commit 37b142c into main Aug 20, 2024
13 checks passed
@mangalaman93 mangalaman93 deleted the harshil-goel/big-int-support branch August 20, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgraph Issue or PR created by an internal Dgraph contributor.
Development

Successfully merging this pull request may close these issues.