Skip to content

NIFI-11823 - fix NUMERIC support in PutBigQuery#7489

Closed
pvillard31 wants to merge 4 commits intoapache:mainfrom
pvillard31:NIFI-11823
Closed

NIFI-11823 - fix NUMERIC support in PutBigQuery#7489
pvillard31 wants to merge 4 commits intoapache:mainfrom
pvillard31:NIFI-11823

Conversation

@pvillard31
Copy link
Copy Markdown
Contributor

Summary

NIFI-11823 - fix NUMERIC support in PutBigQuery

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@pvillard31 pvillard31 marked this pull request as draft July 18, 2023 10:58
@pvillard31 pvillard31 marked this pull request as ready for review July 18, 2023 17:24
@turcsanyip turcsanyip self-requested a review August 1, 2023 17:57
Copy link
Copy Markdown
Contributor

@turcsanyip turcsanyip left a comment

Choose a reason for hiding this comment

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

@pvillard31 Thanks for fixing the decimal handling in the BigQuery processor!

I tested it with different numeric types and it works properly except Avro int (pls. see my comment below).

I can also see regression in the testStreamingNoErrorWithDateFormat IT test. Could you please check it? For me it constantly succeeds on main but fails on the PR branch.

@pvillard31
Copy link
Copy Markdown
Contributor Author

I can also see regression in the testStreamingNoErrorWithDateFormat IT test. Could you please check it? For me it constantly succeeds on main but fails on the PR branch.

Good catch, I guess I focused on the tests I added and forgot to re-run this one... after investigation it feels like trying to send a long into a NUMERIC column is not possible with the current code. We may need to convert int/long into a decimal type before switching to the ByteString. For now, I just fixed the test by switching this specific column to INT64 instead of NUMERIC but curious to hear your thoughts about this.

@turcsanyip
Copy link
Copy Markdown
Contributor

Thanks @pvillard31 for your detailed explanation!

I did not check the result in the BigQuery table, just saw that long was successful, and that's why I thought int should work too. Now I understand that none of int, long, float or double can be inserted into a NUMERIC field with the current code because it creates a different binary representation.

What we can do (if we want to support cross-type conversions, e.g. int to numeric) is to convert the Java int, long, etc. values into BigDecimal first, and then use BigDecimalByteStringEncoder (as for the values originally in BigDecimal).

Something like this:

            case BYTES:
                if (value instanceof Integer) {
                    value = new BigDecimal((int) value);
                } else if (value instanceof Long) {
                    value = new BigDecimal((long) value);
                } else if (value instanceof Float || value instanceof Double) {
                    value = new BigDecimal(value.toString());
                }

                if (value instanceof BigDecimal) {
                    if (tableSchema.getFields(field.getIndex()).getType().equals(Type.BIGNUMERIC)) {
                        value = BigDecimalByteStringEncoder.encodeToBigNumericByteString((BigDecimal) value);
                    } else if (tableSchema.getFields(field.getIndex()).getType().equals(Type.NUMERIC)) {
                        value = BigDecimalByteStringEncoder.encodeToNumericByteString((BigDecimal) value);
                    }
                }

@pvillard31
Copy link
Copy Markdown
Contributor Author

Thanks @turcsanyip - I went ahead with the changes your suggested

Copy link
Copy Markdown
Contributor

@turcsanyip turcsanyip left a comment

Choose a reason for hiding this comment

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

Thanks @pvillard31!

+1 LGTM
Merging to main and 1.x.

@asfgit asfgit closed this in b056bf8 Aug 5, 2023
asfgit pushed a commit that referenced this pull request Aug 5, 2023
This closes #7489.

Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
exceptionfactory pushed a commit to exceptionfactory/nifi that referenced this pull request Aug 14, 2023
This closes apache#7489.

Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
exceptionfactory pushed a commit to exceptionfactory/nifi that referenced this pull request Aug 14, 2023
This closes apache#7489.

Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
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