[FSTORE-414] feature_view.create_train_test_split returns empty df

[davitbzh]

This PR adds/fixes/changes...
Updated converting to epoch milliseconds in spark engine and use util function to be consistent

JIRA Issue: -

Priority for Review: -

Related PRs: -

How Has This Been Tested?

• Unit Tests
• Integration Tests
• Manual Tests on VM

Checklist For The Assigned Reviewer:

- [ ] Checked if merge conflicts with master exist
- [ ] Checked if stylechecks for Java and Python pass
- [ ] Checked if all docstrings were added and/or updated appropriately
- [ ] Ran spellcheck on docstring
- [ ] Checked if guides & concepts need to be updated
- [ ] Checked if naming conventions for parameters and variables were followed
- [ ] Checked if private methods are properly declared and used
- [ ] Checked if hard-to-understand areas of code are commented
- [ ] Checked if tests are effective
- [ ] Built and deployed changes on dev VM and tested manually
- [x] (Checked if all type annotations were added and/or updated appropriately)

[tdoehmen]

I wonder what was the initial problem here. Was it that the conversion didn't work for string-based date columns?
I'm afraid using the convert_event_time_to_timestamp function would lead to inconstistencies between python and java (see https://github.com/logicalclocks/feature-store-api/blob/master/java/src/main/java/com/logicalclocks/hsfs/engine/SparkEngine.java#L309)

[kennethmhc]

the udf should be able to handle "date", "timestamp" as well, so we do not need the if-clause.

[kennethmhc]

I wonder what was the initial problem here. Was it that the conversion didn't work for string-based date columns? I'm afraid using the convert_event_time_to_timestamp function would lead to inconstistencies between python and java (see https://github.com/logicalclocks/feature-store-api/blob/master/java/src/main/java/com/logicalclocks/hsfs/engine/SparkEngine.java#L309)

The problem was that we expect event time column has precision to second and in the backend we will multiple the value by 1000 for comparison. However, Davit use a column which is in millisecond, so the comparison did not work properly.

@davitbzh Like Till said, we should handle millisecond value in Java client as well.

[kennethmhc]

Is this change necessary?

[davitbzh]

If we eventually will support only 10 digit timestamp yes

[kennethmhc]

But now we should any timestamp in second right?

[kennethmhc]

can use util.convert_event_time_to_timestamp directly here.

[davitbzh]

Well I don't want to put this part in util.convert_event_time_to_timestamp

            # for backward compatibility
            if isinstance(event_time, int) and len(str(event_time)) == 13:
                event_time = int(event_time / 1000)

[kennethmhc]

in util.py, we have

    elif isinstance(event_time, int):
        if event_time == 0:
            raise ValueError("Event time should be greater than 0.")
        # jdbc supports timestamp precision up to second only.
        if len(str(event_time)) < 13:
            event_time = event_time * 1000
        return event_time

so, below is redundant.

if isinstance(event_time, int) and len(str(event_time)) == 13:
                event_time = int(event_time / 1000)

[davitbzh]

honestly this is wrong:

        if len(str(event_time)) < 13:
            event_time = event_time * 1000

but I don't know how to replace without braking API.

If we don't use this then how do we guarantee that even_time will have seconds granularity?

if isinstance(event_time, int) and len(str(event_time)) == 13:
                event_time = int(event_time / 1000)

[davitbzh]

I can check either 10 or 13?

[kennethmhc]

LGTM！