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

Question about max byte slice #341

Closed
torwig opened this issue Jan 15, 2024 · 6 comments · Fixed by #342
Closed

Question about max byte slice #341

torwig opened this issue Jan 15, 2024 · 6 comments · Fixed by #342
Labels
bug Something isn't working

Comments

@torwig
Copy link
Contributor

torwig commented Jan 15, 2024

Hi, I see the following comment (https://github.com/hamba/avro/blob/main/config.go#L49C10-L49C10)

// MaxByteSliceSize is the maximum size of `bytes` or `string` types the Reader will create, defaulting to 1MiB.
// If this size is exceeded, the Reader returns an error. This can be disabled by setting a negative number.
MaxByteSliceSize int

And the function here (https://github.com/hamba/avro/blob/main/config.go#L49C10-L49C10):

func (c *frozenConfig) getMaxByteSliceSize() int {
	size := c.config.MaxByteSliceSize
	if size == 0 {
		return maxByteSliceSize
	}
	return size
}

And here we have a constant (https://github.com/hamba/avro/blob/main/config.go#L10C1-L11C37):
const maxByteSliceSize = 1024 * 1024

My question is: 1024*1024 it's not equal to 1 MiB. Is there a typo in the configuration parameter or typo in the maxByteSliceSize value? Or there is somewhere multiplication by 1024 before using it?

BTW, here (https://github.com/hamba/avro/blob/main/reader.go#L227) I see the usage of the function as is, without any multiplication:

        size := int(r.ReadLong())
   .................
        if max := r.cfg.getMaxByteSliceSize(); max > 0 && size > max {
		fnName := "Read" + strings.ToTitle(op)
		r.ReportError(fnName, "size is greater than `Config.MaxByteSliceSize`")
		return nil
	}

Or ReadLong gets the size in kilobytes?

Thank you for any explanation.

@nrwiersma nrwiersma added the bug Something isn't working label Jan 15, 2024
@nrwiersma
Copy link
Member

This is a bug in maxByteSliceSize. Thanks for catching this.

@hhromic
Copy link
Contributor

hhromic commented Jan 17, 2024

My question is: 1024*1024 it's not equal to 1 MiB. Is there a typo in the configuration parameter or typo in the maxByteSliceSize value? Or there is somewhere multiplication by 1024 before using it?

I know this was recently fixed, but I got very curious about this statement. To my understanding, 1024*1024 is equal to 1 MiB. The PR fixing this replaced 1024*1024 with 1_048_576, but both expressions are equal.

fmt.Println(1024*1024 == 1_048_576)
// Output: true

So, I can't wrap my head around what was fixed by it 🙈.

@torwig
Copy link
Contributor Author

torwig commented Jan 17, 2024

@hhromic Yes, indeed equal. I understood it wrong and persuaded others.

@torwig
Copy link
Contributor Author

torwig commented Jan 17, 2024

My apologies, gentlemen.

@hhromic
Copy link
Contributor

hhromic commented Jan 17, 2024

Ah, peace of mind restored! 😄.
I still liked the constant renaming, it is better now IMHO 👍.

@nrwiersma
Copy link
Member

No harm no foul. i could have scrutinised this more, but didn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

3 participants