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

Small bit of simplification #397

Merged
merged 9 commits into from
Nov 29, 2023
Merged

Conversation

philjdf
Copy link
Contributor

@philjdf philjdf commented Nov 23, 2023

  1. Noticed an unused parameter elementType in the constructor of LogicalColumnStream, and also that the Buffer etc members of that class are unused except at the leaf subclass levels, so this looks like it should be composition rather than inheritance.
  2. Noticed LogicalColumnReader<TPhysical, TLogical, TElement> has TPhysical and TLogical generic type args only being used at construction time, so move those types to a static factory method. Also rename it to LogicalColumnReaderBatch to avoid the clash with the base class.
  3. Now there doesn't seem much point in having LogicalColumnReaderBatch, merge it with its parent class LogicalColumnReader. So instead of having sealed LogicalColumnReader<TPhysical, TLogical, TElement> : abstract LogicalColumnReader<TElement> : abstract LogicalColumnReader we've got sealed LogicalColumnReader<TElement> : abstract LogicalColumnReader.
  4. Same on the LogicalColumnWriter side.
  5. Move the LogicalColumnStreamBuffer creation inside the LogicalBatchReader/WriterFactory classes, and it turns out there's already a LogicalStreamBuffers<TPhysical> class there which we can use instead.

@philjdf philjdf changed the title Small bit of simplification, maybe Small bit of simplification Nov 29, 2023
@philjdf philjdf marked this pull request as ready for review November 29, 2023 00:24
Copy link
Contributor

@adamreeve adamreeve left a comment

Choose a reason for hiding this comment

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

This looks great thanks Phil!

@adamreeve adamreeve merged commit 41e3237 into G-Research:master Nov 29, 2023
28 checks passed
@philjdf philjdf deleted the BitOfSimplification branch November 30, 2023 00:09
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.

None yet

2 participants