Skip to content

NIFI-14491 Support other types in CreateBoxFileMetadataInstance#9891

Closed
awelless wants to merge 1 commit intoapache:mainfrom
awelless:NIFI-14491_Support_other_types_in_CreateBoxFileMetadataInstance
Closed

NIFI-14491 Support other types in CreateBoxFileMetadataInstance#9891
awelless wants to merge 1 commit intoapache:mainfrom
awelless:NIFI-14491_Support_other_types_in_CreateBoxFileMetadataInstance

Conversation

@awelless
Copy link
Copy Markdown
Contributor

@awelless awelless commented Apr 22, 2025

Summary

NIFI-14491
Adding support for numeric, date and array types in CreateBoxFileMetadataInstance.
Adding support for date type in UpdateBoxFileMetadataInstance.

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 21

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

@awelless awelless force-pushed the NIFI-14491_Support_other_types_in_CreateBoxFileMetadataInstance branch 2 times, most recently from 0acbf47 to d9064a4 Compare April 23, 2025 08:59
@awelless awelless marked this pull request as ready for review April 23, 2025 08:59
@awelless awelless changed the title [WIP] NIFI-14491 Support other types in CreateBoxFileMetadataInstance NIFI-14491 Support other types in CreateBoxFileMetadataInstance Apr 23, 2025
@awelless awelless force-pushed the NIFI-14491_Support_other_types_in_CreateBoxFileMetadataInstance branch from d9064a4 to 9dab608 Compare April 23, 2025 10:56
Copy link
Copy Markdown

@sfc-gh-rpaulin sfc-gh-rpaulin left a comment

Choose a reason for hiding this comment

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

This is a good addition @awelless. Provided a few suggestions but this looks like the behavior a user would expect for each of these types.

final String path = "/" + fieldName;

if (isNumber(fieldType)) {
metadata.add(path, record.getAsDouble(fieldName));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since isNumber is checking up to BigInteger and BigDecimal (arbitrary precision) it's possible that a number larger than double precision will be retrieved. Given box only accepts up to double precision this seems appropriate. Just wanted to make sure we're calling out the preference that we are opting for being more accepting of numbers being rounded or set to positive or negative infinity over throwing an error. Agree with the approach but wanted to call out the design decision.

"author": "Jones"
"author": "Jones",
"int": 1,
"double": 1.234,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might be good to add a large number to demonstrate we are preferring rounding over erroring when numbers requiring more than 64 bits.

Copy link
Copy Markdown
Contributor Author

@awelless awelless Apr 23, 2025

Choose a reason for hiding this comment

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

Good point. Added a value in the test presenting that scenario

@awelless awelless force-pushed the NIFI-14491_Support_other_types_in_CreateBoxFileMetadataInstance branch from 9dab608 to efcaafa Compare April 23, 2025 13:04
}

private boolean isDate(final RecordFieldType fieldType) {
return RecordFieldType.DATE.equals(fieldType);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
return RecordFieldType.DATE.equals(fieldType);
return RecordFieldType.DATE.equals(fieldType) || RecordFieldType.TIMESTAMP.equals(fieldType);

There is also a timestamp record type, would this be picked up by date or should it be also added in?

Copy link
Copy Markdown
Contributor Author

@awelless awelless Apr 24, 2025

Choose a reason for hiding this comment

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

Box supports dates only. I.e. the time part is 00:00:00.000. I find that treating timestamps as a date is quite risky, as we'll have to drop the time.

If someone wants to put timestamp as a date, they should do the conversion themselves.

@awelless awelless force-pushed the NIFI-14491_Support_other_types_in_CreateBoxFileMetadataInstance branch from efcaafa to 42d4892 Compare April 24, 2025 08:04
TomaszK-stack pushed a commit to TomaszK-stack/nifi that referenced this pull request May 5, 2025
Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com>

This closes apache#9891.
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.

4 participants