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

feat: respect iox::column_type::field metadata when mapping query #200

Merged
merged 23 commits into from
Dec 6, 2024

Conversation

NguyenHoangSon96
Copy link
Contributor

@NguyenHoangSon96 NguyenHoangSon96 commented Nov 21, 2024

Closes #

Proposed Changes

Response from the query will now respect the iox::column_type::field metadata

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Nov 21, 2024
@NguyenHoangSon96 NguyenHoangSon96 removed the request for review from bednar November 21, 2024 10:45
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 90.38462% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.71%. Comparing base (d098ff7) to head (fcca2df).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../v3/client/internal/VectorSchemaRootConverter.java 88.70% 1 Missing and 6 partials ⚠️
...a/com/influxdb/v3/client/internal/TypeCasting.java 81.81% 0 Missing and 2 partials ⚠️
...fluxdb/v3/client/internal/NanosecondConverter.java 96.15% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
+ Coverage   89.59%   89.71%   +0.12%     
==========================================
  Files          17       18       +1     
  Lines         903      963      +60     
  Branches      137      150      +13     
==========================================
+ Hits          809      864      +55     
- Misses         38       39       +1     
- Partials       56       60       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@bednar bednar left a comment

Choose a reason for hiding this comment

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

The VectorSchemaRootConverter has two similar methods: toPointValues and setFieldWithMetaType.

What do you think about reusing the inner part of getArrayObjectFromVectorSchemaRoot in toPointValues? We should prepare a helper method to obtain the correctly typed value for each column and reuse this in both methods.

@NguyenHoangSon96
Copy link
Contributor Author

NguyenHoangSon96 commented Nov 27, 2024

The VectorSchemaRootConverter has two similar methods: toPointValues and setFieldWithMetaType.

What do you think about reusing the inner part of getArrayObjectFromVectorSchemaRoot in toPointValues? We should prepare a helper method to obtain the correctly typed value for each column and reuse this in both methods.

So with this suggestion, we will not need setFieldWithMetaType, right?
I still don't understand how we can make a function that can be used in both toPointValues and getArrayObjectFromVectorSchemaRoot because one set data for an array, and one set data for a PointValues 🤔

@bednar
Copy link
Member

bednar commented Nov 27, 2024

The VectorSchemaRootConverter has two similar methods: toPointValues and setFieldWithMetaType.
What do you think about reusing the inner part of getArrayObjectFromVectorSchemaRoot in toPointValues? We should prepare a helper method to obtain the correctly typed value for each column and reuse this in both methods.

So with this suggestion, we will not need setFieldWithMetaType, right? I still don't understand how we can make a function that can be used in both toPointValues and getArrayObjectFromVectorSchemaRoot because one set data for an array, and one set data for a PointValues 🤔

We can have something like (not production ready, just quick snippet ;)):

    private static Object getMappedValue(String valueType, String metaType, Object value, String fieldName, Field field) {
        if ("field".equals(valueType)) {
            switch (metaType) {
                case "iox::column_type::field::integer":
                case "iox::column_type::field::uinteger":
                    if (value instanceof Number) {
                        return TypeCasting.toLongValue(value);
                    }
                case "iox::column_type::field::float":
                    if (value instanceof Number) {
                        return TypeCasting.toDoubleValue(value);
                    }
                case "iox::column_type::field::string":
                    if (value instanceof Text || value instanceof String) {
                        return TypeCasting.toStringValue(value);
                    }
                case "iox::column_type::field::boolean":
                    if (value instanceof Boolean) {
                        return TypeCasting.toBoolValue(value);
                    }
                default:
                    return value;
            }
        } else if ("timestamp".equals(valueType) || Objects.equals(fieldName, "time")) {
            return NanosecondConverter.getTimestampNano(value, field);
        } else if ("tag".equals(valueType)) {
            return TypeCasting.toStringValue(value);
        } else {
            return value;
        }
    }

and this can be reused:

Object mappedValue = getMappedValue(valueType, metaType, value, fieldName, field);
row.add(mappedValue);

and with little refactoring also in toPointValues.

@NguyenHoangSon96
Copy link
Contributor Author

The VectorSchemaRootConverter has two similar methods: toPointValues and setFieldWithMetaType.
What do you think about reusing the inner part of getArrayObjectFromVectorSchemaRoot in toPointValues? We should prepare a helper method to obtain the correctly typed value for each column and reuse this in both methods.

So with this suggestion, we will not need setFieldWithMetaType, right? I still don't understand how we can make a function that can be used in both toPointValues and getArrayObjectFromVectorSchemaRoot because one set data for an array, and one set data for a PointValues 🤔

We can have something like (not production ready, just quick snippet ;)):

    private static Object getMappedValue(String valueType, String metaType, Object value, String fieldName, Field field) {
        if ("field".equals(valueType)) {
            switch (metaType) {
                case "iox::column_type::field::integer":
                case "iox::column_type::field::uinteger":
                    if (value instanceof Number) {
                        return TypeCasting.toLongValue(value);
                    }
                case "iox::column_type::field::float":
                    if (value instanceof Number) {
                        return TypeCasting.toDoubleValue(value);
                    }
                case "iox::column_type::field::string":
                    if (value instanceof Text || value instanceof String) {
                        return TypeCasting.toStringValue(value);
                    }
                case "iox::column_type::field::boolean":
                    if (value instanceof Boolean) {
                        return TypeCasting.toBoolValue(value);
                    }
                default:
                    return value;
            }
        } else if ("timestamp".equals(valueType) || Objects.equals(fieldName, "time")) {
            return NanosecondConverter.getTimestampNano(value, field);
        } else if ("tag".equals(valueType)) {
            return TypeCasting.toStringValue(value);
        } else {
            return value;
        }
    }

and this can be reused:

Object mappedValue = getMappedValue(valueType, metaType, value, fieldName, field);
row.add(mappedValue);

and with little refactoring also in toPointValues.

Got it, thank you for your detail snipped Jakub

@NguyenHoangSon96 NguyenHoangSon96 removed the request for review from bednar November 29, 2024 04:03
Copy link
Member

@bednar bednar left a comment

Choose a reason for hiding this comment

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

@NguyenHoangSon96, thanks for the PR! We’re almost done; there are just a few minor requirements before we can merge it.

Copy link
Member

@bednar bednar left a comment

Choose a reason for hiding this comment

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

@NguyenHoangSon96, thanks again for the PR 👍. There’s one last small requirement, just to make future maintenance easier:

Copy link
Member

@bednar bednar left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bednar bednar merged commit 7fd6c42 into main Dec 6, 2024
9 checks passed
@bednar bednar deleted the feature/respect-ioxcolumn_typefield-metadata branch December 6, 2024 04:57
@bednar bednar added this to the 0.10.0 milestone Dec 6, 2024
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