Filter pipeline support for datatype conversions based on filtered output datatype. #4165
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has been linked to Shortcut Story #24079: DoubleDelta filter typecheck should account for output type of FloatScaleFilter. |
davisp
reviewed
Jul 12, 2023
shaunrd0
force-pushed
the
smr/sc-24079/filter-output-type
branch
from
9ca7ca7 to
069855c
Compare
KiterLuc
reviewed
Jul 13, 2023
tiledb/sm/filter/filter_pipeline.cc
Outdated
| @@ -221,6 +259,11 @@ Status FilterPipeline::filter_chunks_forward( | |||
| &output_metadata, | |||
| &output_data)); | |||
|
|
|||
| // Final tile type will be the output type of last filter in pipeline. | |||
There was a problem hiding this comment.
Can't we do this in the writer when constructing the tile? I'd like to avoid changing the type after the tile was created.
tiledb/sm/filter/filter_pipeline.cc
Outdated
| if (last_filter) { | ||
| void* output_chunk_buffer = | ||
| static_cast<char*>(tile->data()) + chunk_data.chunk_offsets_[i]; | ||
| RETURN_NOT_OK(output_data.set_fixed_allocation( | ||
| output_chunk_buffer, chunk.unfiltered_data_size_)); | ||
| reader_stats->add_counter( | ||
| "read_unfiltered_byte_num", chunk.unfiltered_data_size_); | ||
| // Restore tile datatype to it's initial schema value. |
There was a problem hiding this comment.
I think the tile created by the reader should have the right type from the get go. My understanding is that the filter pipeline will use temporary buffers until we reach the last filter, at which point we'll copy to the final tile. Am I incorrect? I don't think we should change the tile type after it's been created.
davisp
previously requested changes
Jul 14, 2023
shaunrd0
commented
Jul 17, 2023
shaunrd0
force-pushed
the
smr/sc-24079/filter-output-type
branch
from
3076f33 to
71af9fc
Compare
KiterLuc
reviewed
Jul 19, 2023
KiterLuc
approved these changes
Jul 19, 2023
shaunrd0
force-pushed
the
smr/sc-24079/filter-output-type
branch
from
eaaa2a6 to
c36371d
Compare
ihnorton
reviewed
Jul 28, 2023
This reverts commit c5ba20c. + Clang format after merge
ihnorton
approved these changes
Jul 28, 2023
ihnorton
changed the title
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Passes filter output datatype to next filter(s) in pipeline, adding support for type conversions within a pipeline based on filter output datatype. Also refactors pipeline validation, adding
Filter::accepts_input_datatypeandFilter::output_datatypefrom #4057. Deriving filters can override these methods if they are restricted to certain input datatypes and / or convert tile datatype when the filter is applied.TYPE: IMPROVEMENT
DESC: Filter pipeline support for datatype conversions based on filtered output datatype.