Skip to content

Add period multiplication times longs#5635

Open
chipkent wants to merge 1 commit intodeephaven:mainfrom
chipkent:period_long_multiply
Open

Add period multiplication times longs#5635
chipkent wants to merge 1 commit intodeephaven:mainfrom
chipkent:period_long_multiply

Conversation

@chipkent
Copy link
Copy Markdown
Member

Add period multiplication times longs.

@chipkent
Copy link
Copy Markdown
Member Author

The feature was pulled from #5509 into a separate PR because of ongoing discussions at #5509 (comment).

There are basically two camps.

  1. Purist devs that don't want the feature (@rcaudy @devinrsmith )
  2. Pragmatic users that see the use case pop up in reality (@rbasralian @chipkent )

@alexpeters1208
Copy link
Copy Markdown
Contributor

alexpeters1208 commented Jun 18, 2024

I would also like to have it. I tested the original PR and lack of support for this surprised me. It would not surprise me if I was a Java guru, but I don't think that's the standard of surprise that users should be held to. IE, "it shouldn't surprise you because you should know what's going on" does not sound like a reasonable argument to me.

Copy link
Copy Markdown
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

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

We have no other methods that do this. Doing it here sets a bad precedent that we will hold the user's hand on this sort of type adjustment. It's simply not a good idea. If we do want something like this, do it within JPY for all long to int conversions.

I do not want to merge this.

Copy link
Copy Markdown
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

I'll say the same thing I did in the other review:

If users are frequently finding themselves with the issue "I've got a logical int-column, but the column type says it's a long" that's the root of the problem that we should address. Why is the column typed wider than the user expected / wanted?

Similarly for the request "I've got a BigInteger, why can't I use that to multiply a Period?", my argument against that would be the same as my argument against providing a long version.

@devinrsmith devinrsmith added this to the 3. Triage milestone Jun 22, 2024
@devinrsmith
Copy link
Copy Markdown
Member

devinrsmith commented Jun 22, 2024

I don't know if there would be any inherent advantage over a view formula, but being able to re-interpret a column from a long to an int (with strict safety checks) via io.deephaven.engine.table.ColumnSource#reinterpret might be nice?

@chipkent
Copy link
Copy Markdown
Member Author

#6110 can help convert long to int in a safe way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants