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

Adding support for indexing Date data types #129

Merged
merged 7 commits into from
Sep 1, 2022
Merged

Adding support for indexing Date data types #129

merged 7 commits into from
Sep 1, 2022

Conversation

Adricu8
Copy link
Contributor

@Adricu8 Adricu8 commented Aug 30, 2022

Description

Adding support for indexing Datetime data type in qbeast-spark.
In reference to issue.

Type of change

It is an enhancement that would allow for Datetime data type to be indexed without the need of a workaround like casting the value to a Long type or similar.
Spark has two date data types, we could add support for both.

More elaborated explanation and guidelines regarding the addition of new types here

Checklist:

  • New feature / bug fix has been committed following the Contribution guide.
  • Add comments to the code (make it easier for the community!).
  • Change the documentation.
  • Add tests.
  • Your branch is updated to the main branch (dependent changes have been merged).

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • TransformerTest: I have added a test to check the correct behavior of LinearTransformation with Timestamps
  • TransformerIndexingTest: I have added a test to ensure timestamps read and write correctly with spark

Test Configuration:

  • Spark Version: 3.1.1
  • Hadoop Version: 3.2
  • Running Locally on M1

@Adricu8 Adricu8 requested a review from osopardo1 August 30, 2022 15:22
@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #129 (2f27554) into main (cc3df2c) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 2f27554 differs from pull request most recent head 68ed1ba. Consider uploading reports for the commit 68ed1ba to get more accurate results

@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   91.68%   91.77%   +0.08%     
==========================================
  Files          62       62              
  Lines        1432     1446      +14     
  Branches      106      108       +2     
==========================================
+ Hits         1313     1327      +14     
  Misses        119      119              
Impacted Files Coverage Δ
...rc/main/scala/io/qbeast/core/model/QDataType.scala 100.00% <100.00%> (ø)
...o/qbeast/core/transform/LinearTransformation.scala 89.33% <100.00%> (+0.60%) ⬆️
...a/io/qbeast/core/transform/LinearTransformer.scala 100.00% <100.00%> (ø)
...cala/io/qbeast/core/transform/Transformation.scala 66.66% <100.00%> (+16.66%) ⬆️
...ala/io/qbeast/spark/utils/SparkToQTypesUtils.scala 92.30% <100.00%> (+1.39%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@osopardo1
Copy link
Member

Well done! Just a few more comments/questions:

  1. We could put something more user-friendly in the title of the PR. For example: Support for indexing Date columns. Then everyone can understand what this is about. Because Transformer and QDataTypes are something more dependent on implementation rather than functionality. 👨‍💻
  2. I guess for the implementation we are only adding Timestamp. But you have intentions to add a Date as well, right?

@eavilaes eavilaes linked an issue Aug 31, 2022 that may be closed by this pull request
@Adricu8 Adricu8 changed the title changed Transformer related files and QDataTypes to support Timestamps Adding support for indexing Date data types Aug 31, 2022
@osopardo1 osopardo1 added the type: enhancement Improvement of existing feature or code label Aug 31, 2022
@Adricu8 Adricu8 marked this pull request as ready for review August 31, 2022 15:00
@osopardo1
Copy link
Member

Looks good!

@Adricu8 Adricu8 merged commit 5d86c47 into Qbeast-io:main Sep 1, 2022
@Adricu8 Adricu8 deleted the randomize branch September 1, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Improvement of existing feature or code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for indexing type Date
2 participants