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 for temporal types in graphical query builder #191

Closed
vogti opened this issue Feb 9, 2020 · 14 comments · Fixed by #339
Closed

Support for temporal types in graphical query builder #191

vogti opened this issue Feb 9, 2020 · 14 comments · Fixed by #339
Assignees
Labels
A-db Area: DB A-ui Area: UI C-enhancement Category: An issue proposing an enhancement E-good-first-issue P-low Priority: Low

Comments

@vogti
Copy link
Member

vogti commented Feb 9, 2020

The graphical query builder only supports building conditions for numerical and character types. Adding support for temporal types requires extending to the statistics manager in Polypheny-DB (org/polypheny/db/statistic/StatisticsManager.java) and the corresponding component in the Polypheny-UI.

@vogti vogti added C-enhancement Category: An issue proposing an enhancement A-ui Area: UI A-db Area: DB E-medium P-low Priority: Low labels Feb 9, 2020
@vogti vogti changed the title Add support for temporal types in graphical query builder Support for temporal types in graphical query builder Mar 14, 2021
@earthshakira
Copy link
Contributor

Hi! @vogti can I work on this issue ?

@vogti
Copy link
Member Author

vogti commented Mar 15, 2021

Yes, of course! It would be great if you can take care of this.

@vogti
Copy link
Member Author

vogti commented Mar 17, 2021

@earthshakira I forgot to mention something (and hope you are not already frustrated due to this). You might be wondering why the system does not calculate statistics for newly added tables. This is currently disabled at default. We are currently working on a new workload monitoring which also integrates the statistics module more efficiently. For the time being you can either enable Config -> Dynamic Querying -> statistics/passiveTracking or restart Polypheny-DB (statistics/statisticsOnStartup is enabled on default).

To make it easier for you to trigger a recalculation, I have attached a patch which adds a button to Monitoring -> Statistics which allows to recalculate all statistics.

recalculateStatisticsButton.patch.zip

@earthshakira
Copy link
Contributor

@vogti Thanks for the patch helps a lot ✌

@vogti
Copy link
Member Author

vogti commented Apr 2, 2021

Hi @earthshakira, is everything working fine? If you have any issues or need help getting started, please do not hesitate to either ask here or open a thread in the discussion forum.

@earthshakira
Copy link
Contributor

@vogti sorry, I was busy with some college work so it's taking extra time, extremely sorry for the delay

@vogti
Copy link
Member Author

vogti commented Apr 2, 2021

No worries. There is no need to hurry. I just wanted to ask if you need any assistance. I am already looking forward to your pull request :)

@earthshakira
Copy link
Contributor

Hi @vogti thanks for the patch, it's really really helpful.

For the temporal types I have chosen PolyTypeFamily.DATE , PolyTypeFamily.TIMESTAMP , PolyTypeFamily.TIME
All of these use the Numeric format so for now I have Inherited the 'NumericalStatisticColumn' as it works for numbers.

It's working for most of the part, The problem it's having is since all of these types give a Long value for toString(), I was trying to use DateTimeStringUtils.longToAdjustedString(Number,PolyType) this helps in some parts like the InformationTable given below

Temporal Fields in the Statistics Manager

but it doesn't work for cases like the API call /allStatistics

bar: {columnType: "temporal", min: "18724", max: "18724", schema: "public", table: "temporal_tests",}
foo: {columnType: "temporal", min: "1618056000000", max: "1618056000000", schema: "public", table: "temporal_tests",}
foobar: {columnType: "temporal", min: "43200000", max: "43200000", schema: "public", table: "temporal_tests",}

since this directly serializes the map

also in other parts of the UI where this is used since we don't know the exact type(DATE,TIMESTAMP or TIME) is not known, I couldn't find a good way how it would be converted to time strings in UI
Though we can use length of the string as an indicator, it seemed like a bad solution.

Note:
I also tried to use DateTimeStringUtils.longToAdjustedString in the reevalueateTemporalColumn, but as it uses the generic argument, I'm not able to return string.

Please do help me.

Thanks again for the Recalculate Button.

@datomo
Copy link
Member

datomo commented Apr 6, 2021

Hey @earthshakira
I think the easiest way to fix your problem would be to add a field in your temporalStatisticColumn, where you save the type (time or timestamp) as string.
When you annotate that field with @expose it gets serialized automaticly when /allStatistics is called and you can test in the frontend what this fields value is.
The same approach can you use if you want to save the time or timestamp in any other format.

I hope I understood your question correctly and this helps.
If you need more information, feel free to report back

@earthshakira
Copy link
Contributor

Hi! @datomo,

I tried implementing the solution, it works fine from the backend, but it's adding additional complexity on the UI side as we need to include the methods for parsing the datetime strings.
I was thinking, is we could modify the toString or Encode method from the backend then it would reduce the parsing complexity on the front end
Also, dealing with string times is much easier than Long in Angular, so that ways dealing with the datetimes could be simplified a lot as inbuilt functions to directly parse the ISO date or datetime

is this a bad way to deal with this issue ? Please Guide me on this.

Thanks

@datomo
Copy link
Member

datomo commented Apr 17, 2021

Sadly this does not really work, when using Gson, as we do, to seralize the map with all the StatisticColumns.
For Gson you would need to implement your own JSONSerializer if you want to control completly how each field is serialized.
If you feel up to the task here is an example:
https://howtodoinjava.com/gson/custom-serialization-deserialization/ (BooleanSerializer.java)
But you would not need to implement it for the whole TemporalStatisticColumn but only for your special types.

Those you could then enable by adding them in the Crud class to the gsonExpose like this:
GsonBuilder .registerTypeAdapter(TimeDate.class, new TimeDateSerializer()).registerTypeAdapter([...], [...]).create

But you could try a different approach.
Maybe you could just add additional fields to the TemporalStatisticColumn, which for example hold the timestamp or time in a String format, additionally to the "normal" type of the field.
This gets adds a lot of redundency but would work as well. (but wouldn't be as efficent and pretty as the previous solution)
Like so:

class TempralStatisticColumn{
...
@Expose
Timestamp max;

@Expose
String maxPrint

In the frontend you would then be able to test and access the "print" field if you have one of your special columns.

I hope this helps, feel free to report back.

@earthshakira
Copy link
Contributor

Hi!, Please check #339, since the temporal types are always having a string representation, I'm changing their representation from Number into TimeStamp format using the util helper when reevaluateTemporalStatistic, so it works well currently for the hsqldb source. Could it cause some issues for other sources ?

@datomo
Copy link
Member

datomo commented May 29, 2021

While I think the the transformation to TimeStamp should not be a problem, you would need to take some precautions when creating your TemporalStasticColumns from the new data. Not all input data of a PolyType has to be the same underlying Java type, but you should be able to transform it rather easily. You would need to make sure that you only insert it as Longs, so for one your interal logic works as expected and for another that your transformations work correctly.

@earthshakira
Copy link
Contributor

earthshakira commented Jun 7, 2021

Hi! have implemented the parsing logic, so only Longs get parsed and the other types don't, please check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-db Area: DB A-ui Area: UI C-enhancement Category: An issue proposing an enhancement E-good-first-issue P-low Priority: Low
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants