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

Support BigInteger columns in in-memory Table #7712

Closed
9 tasks done
radeusgd opened this issue Sep 1, 2023 · 6 comments · Fixed by #7715
Closed
9 tasks done

Support BigInteger columns in in-memory Table #7712

radeusgd opened this issue Sep 1, 2023 · 6 comments · Fixed by #7715
Assignees
Labels
-libs Libraries: New libraries to be implemented l-table-column-datatypes p-medium Should be completed in the next few sprints x-new-feature Type: new feature request

Comments

@radeusgd
Copy link
Member

radeusgd commented Sep 1, 2023

Enso supports big integers seamlessly - an Integer is stored as a long if it fits, and if it doesn't, it gets 'promoted' to BigInteger.

However, our Table library only supports LongStorage and does not handle big integers at all.

To achieve parity with the core language, we should allow the table to support big integers as well.

  1. Add a BigIntegerStorage to the in-memory table.
    • We could use the 'compact' form that uses long if the integers are small enough and falls back into BigInteger only for large values, but I think from the perspective of simplicity it may be better to just go for uniform storage.
    • This storage will report as the Decimal Value_Type with scale=0 (meaning no digits after decimal point). scale > 0 support will come as a separate ticket since it would require BigDecimal support which the language does not yet have.
  2. Update the InferredBuilder to promote to BigIntegerStorage if too large integer is encountered.
  3. Ensure existing arithmetic operations are implemented for the big integer storage.
    • And ensure that it is possible to perform operations on mixed types i.e. Integer + Decimal -> Decimal
    • Add tests for these scenarios.
  4. It could be worth to add tests for a round-trip from and to the Postgres database which also has a decimal type.
    • We can upload a table containing big integers and then read it back and check the value.
    • We have to ensure that Column_Fetcher can support big integers when reading Decimal type columns.
@radeusgd radeusgd added p-lowest Should be completed at some point -libs Libraries: New libraries to be implemented l-table-column-datatypes labels Sep 1, 2023
@radeusgd radeusgd self-assigned this Sep 1, 2023
@github-project-automation github-project-automation bot moved this to ❓New in Issues Board Sep 1, 2023
@radeusgd radeusgd added the x-new-feature Type: new feature request label Sep 1, 2023
@jdunkerley jdunkerley added p-medium Should be completed in the next few sprints and removed p-lowest Should be completed at some point labels Sep 4, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Sep 4, 2023
@radeusgd radeusgd moved this from 📤 Backlog to 🔧 Implementation in Issues Board Sep 4, 2023
@enso-bot
Copy link

enso-bot bot commented Sep 6, 2023

Radosław Waśko reports a new STANDUP for yesterday (2023-09-05):

Progress: Fixing comparisons, removing checks preventing BigInt ops. Testing. Adding edge case tests and handler to delete_rows - I have only now noticed a NULL edge case that needed handling. Changed IN to EXISTS as apparently that is more optimizer friendly. Discussions on problem handling. It should be finished by 2023-09-07.

Next Day: Next day I will be working on the same task. Finalize delete_rows. Add tests for BigInt operations and ensure all is working. Remove obsolete warnings.

@enso-bot
Copy link

enso-bot bot commented Sep 6, 2023

Radosław Waśko reports a new STANDUP for today (2023-09-06):

Progress: Fighting with a weird stacktrace bug found in delete_rows tests. Created an issue in oracle/graal and a PR with a fix. That fixes an inconsistency but our bug is more complex. Created a workaround for the bug. Found issue where table naming scheme was not used and an internal table was exceeding the table name length limit - fixed it. Updated the PR - ready for re-review. Updating BigInteger tests, adding tests for handling of arithmetic. Fixing edge cases. Many tests are passing but still some failures to address. It should be finished by 2023-09-07.

Next Day: Next day I will be working on the same task. Fix the remaining tests and prepare the PR for re-review.

@enso-bot
Copy link

enso-bot bot commented Sep 7, 2023

Radosław Waśko reports a new STANDUP for today (2023-09-07):

Progress: finally delete rows is merged. Prepared the big integers PR ready for review. Forgot about one small test - will fix on Monday. Did the book club. Reviews. It should be finished by 2023-09-07.

Next Day: Next day I will be working on the #7461 task. fix remaining test. look into next tasks

@radeusgd radeusgd moved this from 🔧 Implementation to 👁️ Code review in Issues Board Sep 11, 2023
@enso-bot
Copy link

enso-bot bot commented Sep 11, 2023

Radosław Waśko reports a new 🔴 DELAY for today (2023-09-11):

Summary: There is 6 days delay in implementation of the Support BigInteger columns in in-memory Table (#7712) task.
It will cause 0 days delay for the delivery of this weekly plan.

Half of delay magnitude stems from the fact that working on the feature crossed my 1day time-off and the weekend.

Delay Cause: The small test case turned out to be quite a rabbit hole, but we decided that since we support BigInteger columns in-memory, we also want the integration with Database to correctly download such columns as BigInteger, wherever possible. Some more time needed to finish it.

@enso-bot
Copy link

enso-bot bot commented Sep 11, 2023

Radosław Waśko reports a new STANDUP for today (2023-09-11):

Progress: The small test case turned out to be quite a rabbit hole, but we decided that since we support BigInteger columns in-memory, we also want the integration with Database to correctly download such columns as BigInteger, wherever possible. Working on that feature (adding tests for it, updating column fetcher, statement setter, updating the type mappings). Meetings, some pair programming with James on Managed Resources. Catching up. It should be finished by 2023-09-13.

Next Day: Next day I will be working on the same task. Continue fixing the biginteger round-trip example - ensure what set of warnings is sane.

@mergify mergify bot closed this as completed in #7715 Sep 12, 2023
mergify bot pushed a commit that referenced this issue Sep 12, 2023
- Fixes #7354
- And also closes #7712
- Refactors how we handle numeric ops - ensuring that the 'kernels' are placed all in one place and selected based on storage types.
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Sep 12, 2023
@enso-bot
Copy link

enso-bot bot commented Sep 13, 2023

Radosław Waśko reports a new STANDUP for yesterday (2023-09-12):

Progress: Cleanup, addressing comments, got the biginteger PR merged. Reviews. Started looking into how to address the Date.parse format issues - experimenting with adding a sum type with a simple default and customization options. It should be finished by 2023-09-12.

Next Day: Next day I will be working on the #7461 task. implement a prototype and experiment with it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-libs Libraries: New libraries to be implemented l-table-column-datatypes p-medium Should be completed in the next few sprints x-new-feature Type: new feature request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants