METRON-1107 add support for handling epoch dates in seconds or milliseconds #692

ottobackwards · 2017-08-13T04:34:28Z

This PR adds the capability for the Stellar Date functions which take Long times to handle both seconds and milliseconds, so they just handle epoch time.

Testing

run stellar shell and exercise the Stellar date functions using epochconverter.com.

Use both the epoch seconds and millisecond values

In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:

For all changes:

Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?

For code changes:

Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && build_utils/verify_licenses.sh 
```
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via site-book/target/site/index.html:
```
cd site-book
mvn site
```

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.

JonZeolla

Looks good to me, aside from the 10 digit assumption and some minor typos.

JonZeolla · 2017-08-13T22:59:31Z

...-stellar/stellar-common/src/main/java/org/apache/metron/stellar/common/utils/EpochUtils.java

+   *
+   *
+   * </p>
+   * @param canidate The Long value to concider


s/concider/consider/ for the whole file

:%s/x/y/g
will do the whole file :)

I wasn't assuming vi, I was assuming more of a sed-style formatting, plus you only need the g for your canidate typo because it occurs twice on one line. =)

JonZeolla · 2017-08-13T22:59:58Z

...-stellar/stellar-common/src/main/java/org/apache/metron/stellar/common/utils/EpochUtils.java

+   */
+  public static Long ensureEpochMillis(Long canidate) {
+    int length = (int)Math.floor(Math.log10(canidate) + 1);
+    return length == 10 ? canidate * 1000 : canidate;


We should handle inputs of length 1-9 as well.

JonZeolla · 2017-08-13T23:01:41Z

...-stellar/stellar-common/src/main/java/org/apache/metron/stellar/common/utils/EpochUtils.java

+   * Ensures returns the passed value as milliseconds from EPOCH if the value is in seconds.
+   * This is done by looking at the number of digits.
+   * If there are 10, then the value is concidered to be in seconds and will by
+   * muliplited by 1000.


s/muliplited/multiplied/

JonZeolla · 2017-08-13T23:02:44Z

...-stellar/stellar-common/src/main/java/org/apache/metron/stellar/common/utils/EpochUtils.java

+   * @param canidate The Long value to concider
+   * @return A Long value
+   */
+  public static Long ensureEpochMillis(Long canidate) {


s/canidate/candidate/g for this whole file

JonZeolla · 2017-08-13T23:05:36Z

...llar/stellar-common/src/test/java/org/apache/metron/stellar/common/utils/EpochUtilsTest.java

+    Assert.assertTrue(meh.longValue() == EpochUtils.ensureEpochMillis(meh).longValue());
+  }
+
+


Suggest adding some tests for < 10 digit epochs.

JonZeolla · 2017-08-13T23:07:01Z

.../stellar-common/src/test/java/org/apache/metron/stellar/dsl/functions/DateFunctionsTest.java

+    Object result = run("DAY_OF_YEAR(epoch_seconds)");
+    assertEquals(238, result);
+  }
+


Suggest adding tests for an epoch time at or before Sunday, September 9, 2001 1:46:39 AM.

JonZeolla · 2017-08-13T23:09:57Z

Also, I did notice a couple of things when poking around that probably fit best into some new JIRAs that I at least wanted to mention here:

It's somewhat unintuitive that we count starting at zero for MONTH(), unlike WEEK_OF_MONTH, WEEK_OF_YEAR, DAY_OF_MONTH, and DAY_OF_WEEK.
We don't comply with ISO 8601 regarding the first day of the week (We use Sunday, ISO says Monday).

[Stellar]>>> MONTH()
7
[Stellar]>>> MONTH(1502664380)
7
[Stellar]>>> WEEK_OF_MONTH(1502664380)
3
[Stellar]>>> DAY_OF_MONTH(1502664380)
13
[Stellar]>>> WEEK_OF_YEAR(1502664380)
33
[Stellar]>>> DAY_OF_WEEK(1502664380)
1

ottobackwards · 2017-08-14T14:59:27Z

This is due to our time stuff being java Calendar based. We should have a new jira to convert to the new java.time concepts.

Comments from Reviewable

ottobackwards · 2017-08-14T15:01:57Z

metron-stellar/stellar-common/src/main/java/org/apache/metron/stellar/common/utils/EpochUtils.java, line 41 at r1 (raw file):

Previously, JonZeolla (JonZeolla) wrote…

We should handle inputs of length 1-9 as well.

So, the 10/13 rule is pretty standard. If we do this, then we know we are doing seconds->millis for current timestamps.

for < 10, we don't know if it is millis or timestamps, at least that is my thinking. adding a 0 up to 13 for those numbers is equally as bad as just leaving them no?

Comments from Reviewable

ottobackwards · 2017-08-14T15:53:08Z

metron-stellar/stellar-common/src/test/java/org/apache/metron/stellar/dsl/functions/DateFunctionsTest.java, line 248 at r1 (raw file):

Previously, JonZeolla (JonZeolla) wrote…

Suggest adding tests for an epoch time at or before Sunday, September 9, 2001 1:46:39 AM.

can I ask why?

Comments from Reviewable

ottobackwards · 2017-08-14T15:53:34Z

metron-stellar/stellar-common/src/main/java/org/apache/metron/stellar/common/utils/EpochUtils.java, line 36 at r1 (raw file):

Previously, JonZeolla (JonZeolla) wrote…

I wasn't assuming vi, I was assuming more of a sed-style formatting, plus you only need the g for your canidate typo because it occurs twice on one line. =)

one upper

Comments from Reviewable

JonZeolla · 2017-08-14T16:41:43Z

can I ask why?

Because that is 9x 9s in epoch (999999999, assuming seconds).

for < 10, we don't know if it is millis or timestamps, at least that is my thinking. adding a 0 up to 13 for those numbers is equally as bad as just leaving them no?

Well, that's partially true. We know that for 1-3 digits it must be seconds, and for 11-13 digits it must be millis. Perhaps we provide an optional field that specifies millis vs seconds with a default to whatever is more common (seconds) and be done with it? Do we have any core assumptions of using milli elsewhere? Of course this will need an update to a lot of tests to either change to seconds or specify milli.

As far as attempting to appropriately zero-pad it (which I think is what you mentioned but I could be wrong), I'm probably against that because a small mistake could drastically affect the timestamp. To be explicit - 1234567890 (2009) could become 00001234567890 (1970) or 123456789 (1973) could become 0000123456789 (1970). More realistically, this probably isn't a horrible approach either.

ottobackwards · 2017-08-14T17:11:17Z

I'm going to close this. We need more discussion.

add support for handling dates in seconds or milliseconds

b4a76d3

ottobackwards force-pushed the stellar_time_seconds branch from c26b3d8 to b4a76d3 Compare August 13, 2017 04:35

more tests, javadoc and an tweek

186d53e

JonZeolla suggested changes Aug 13, 2017

View reviewed changes

spelling changes per review

96ea877

ottobackwards closed this Aug 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

METRON-1107 add support for handling epoch dates in seconds or milliseconds #692

METRON-1107 add support for handling epoch dates in seconds or milliseconds #692

ottobackwards commented Aug 13, 2017

JonZeolla left a comment

JonZeolla Aug 13, 2017

ottobackwards Aug 14, 2017

JonZeolla Aug 14, 2017

JonZeolla Aug 13, 2017

JonZeolla Aug 13, 2017

ottobackwards Aug 14, 2017

JonZeolla Aug 13, 2017

ottobackwards Aug 14, 2017

JonZeolla Aug 13, 2017 •

edited

JonZeolla Aug 13, 2017

JonZeolla commented Aug 13, 2017 •

edited

ottobackwards commented Aug 14, 2017

ottobackwards commented Aug 14, 2017

ottobackwards commented Aug 14, 2017

ottobackwards commented Aug 14, 2017

JonZeolla commented Aug 14, 2017

ottobackwards commented Aug 14, 2017

		Assert.assertTrue(meh.longValue() == EpochUtils.ensureEpochMillis(meh).longValue());
		}

METRON-1107 add support for handling epoch dates in seconds or milliseconds #692

METRON-1107 add support for handling epoch dates in seconds or milliseconds #692

Conversation

ottobackwards commented Aug 13, 2017

Testing

For all changes:

For code changes:

For documentation related changes:

Note:

JonZeolla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonZeolla Aug 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonZeolla commented Aug 13, 2017 • edited

ottobackwards commented Aug 14, 2017

ottobackwards commented Aug 14, 2017

ottobackwards commented Aug 14, 2017

ottobackwards commented Aug 14, 2017

JonZeolla commented Aug 14, 2017

ottobackwards commented Aug 14, 2017

JonZeolla Aug 13, 2017 •

edited

JonZeolla commented Aug 13, 2017 •

edited