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

Fixes writer max annotation option doesn't work. #260

Merged
merged 2 commits into from
Dec 1, 2021

Conversation

cheqianh
Copy link
Contributor

@cheqianh cheqianh commented Dec 1, 2021

Description:

This PR is working on amazon-ion/ion-python#165 on ion-c layer and the fix of ion-python layer is here.

Step by step fix:

  1. The writer option's max annotation count is set when writer is initialized but never used. For example, I set max annotation count to 100 but it still writes at most 10 annotations.
  2. The root is because the writer always set annotations counts to default (10) when a writer writes the first annotation no matter how much we set for max_annotation_count. Code details is here. Changed it to max_annotation_count as below (line 1099).
  3. Then it raise an IERR_NO_MEMORY error when max_annotation_count is so large. The root is because we only allocate default memory (In this case, it's 1024 bytes) for temp_buffer where the annotation bytes is stored. The code details is here. So I calculated how much bytes we need for annotations:
    p_options->max_annotation_count * sizeof(ION_SYMBOL)
  4. However, above three steps fix the ion-hash issue but fail some ion-c unit test. This is because the temp_buffer is not only used for annotation. For example, ion_writer_text reserved bytes from temp buffer twice. So, for example, if there are 100 bytes for annotations, we need to allocate more than 100 bytes for temp_buffer because it will be used somewhere else too.
  5. So I separate the annotation calculation from the defaults bytes. I allocate p_options->max_annotation_count * sizeof(ION_SYMBOL) + ION_WRITER_TEMP_BUFFER_DEFAULT bytes for temp_buffer (see change below line 278) where it has +ION_WRITER_TEMP_BUFFER_DEFAULT at the end. So we calculate how many bytes annotations need first, then add ION_WRITER_TEMP_BUFFER_DEFAULT for other fields, which solves the issue and passes all tests.
  6. Here might be a minor improvement. Before we allocate 1024 bytes for everything but now we separate annotations and other fields so it doesn't need default to 1024 bytes anymore. I think we can decrease the default value to 512 and add some comments explaining why.

Tests:

This change passes ion-c tests, ion-python tests and ion-hash-python tests.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

tgregg
tgregg previously approved these changes Dec 1, 2021
ionc/ion_writer.c Outdated Show resolved Hide resolved
@cheqianh cheqianh merged commit f7d33b0 into amazon-ion:master Dec 1, 2021
m6w6 added a commit to m6w6/ion-c that referenced this pull request Dec 6, 2021
@m6w6 m6w6 mentioned this pull request Dec 6, 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.

2 participants