You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2020/05/29 13:36:02 UTC
[GitHub] [incubator-iotdb] wshao08 opened a new pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
wshao08 opened a new pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] qiaojialin merged pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
qiaojialin merged pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] jixuan1989 commented on a change in pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290#discussion_r432817911
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1475,7 +1475,15 @@ long parseTimeFormat(String timestampStr) throws SQLParserException {
throw new SQLParserException("input timestamp cannot be empty");
}
if (timestampStr.equalsIgnoreCase(SQLConstant.NOW_FUNC)) {
- return System.currentTimeMillis();
+ String timePrecision = IoTDBDescriptor.getInstance().getConfig().getTimestampPrecision();
+ switch (timePrecision) {
+ case "ns":
+ return System.currentTimeMillis() * 1000_000L;
Review comment:
When users use `now()` and set the precision as `ns` or `ms`, they want to generate dense data. if you you `System.currentTimeMillis()*1000` or `1000_000`, then many timstamps will repeat...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] jixuan1989 commented on a change in pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290#discussion_r432935634
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1475,7 +1475,15 @@ long parseTimeFormat(String timestampStr) throws SQLParserException {
throw new SQLParserException("input timestamp cannot be empty");
}
if (timestampStr.equalsIgnoreCase(SQLConstant.NOW_FUNC)) {
- return System.currentTimeMillis();
+ String timePrecision = IoTDBDescriptor.getInstance().getConfig().getTimestampPrecision();
+ switch (timePrecision) {
+ case "ns":
+ return System.currentTimeMillis() * 1000_000L;
Review comment:
I think `Instant.now()` is a good solution.
Another way is just using the last 6 digits of System.nanoTime(), as in a JVM instance (which means if you do not restart the JVM), System.nanoTime() is keeping increasing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] HTHou commented on a change in pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
HTHou commented on a change in pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290#discussion_r432801939
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1475,7 +1475,15 @@ long parseTimeFormat(String timestampStr) throws SQLParserException {
throw new SQLParserException("input timestamp cannot be empty");
}
if (timestampStr.equalsIgnoreCase(SQLConstant.NOW_FUNC)) {
- return System.currentTimeMillis();
+ String timePrecision = IoTDBDescriptor.getInstance().getConfig().getTimestampPrecision();
+ switch (timePrecision) {
+ case "ns":
+ return System.currentTimeMillis() * 1000_000L;
Review comment:
I'm just a little bit curious why don't you use `System.nanoTime()` here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] wshao08 commented on a change in pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
wshao08 commented on a change in pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290#discussion_r432934908
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1475,7 +1475,15 @@ long parseTimeFormat(String timestampStr) throws SQLParserException {
throw new SQLParserException("input timestamp cannot be empty");
}
if (timestampStr.equalsIgnoreCase(SQLConstant.NOW_FUNC)) {
- return System.currentTimeMillis();
+ String timePrecision = IoTDBDescriptor.getInstance().getConfig().getTimestampPrecision();
+ switch (timePrecision) {
+ case "ns":
+ return System.currentTimeMillis() * 1000_000L;
Review comment:
Use Instant to generate nano seconds instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290#discussion_r432953766
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1475,7 +1475,15 @@ long parseTimeFormat(String timestampStr) throws SQLParserException {
throw new SQLParserException("input timestamp cannot be empty");
}
if (timestampStr.equalsIgnoreCase(SQLConstant.NOW_FUNC)) {
- return System.currentTimeMillis();
+ String timePrecision = IoTDBDescriptor.getInstance().getConfig().getTimestampPrecision();
+ switch (timePrecision) {
+ case "ns":
+ return System.currentTimeMillis() * 1000_000L;
Review comment:
`Instant.getNano()` returns the nanoseconds, but actully it's still millisecond resolution. I'm not sure it's useful here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] Alima777 commented on a change in pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290#discussion_r432927503
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1475,7 +1475,15 @@ long parseTimeFormat(String timestampStr) throws SQLParserException {
throw new SQLParserException("input timestamp cannot be empty");
}
if (timestampStr.equalsIgnoreCase(SQLConstant.NOW_FUNC)) {
- return System.currentTimeMillis();
+ String timePrecision = IoTDBDescriptor.getInstance().getConfig().getTimestampPrecision();
+ switch (timePrecision) {
+ case "ns":
+ return System.currentTimeMillis() * 1000_000L;
Review comment:
I agree with @qiaojialin and @jixuan1989 both. System.nanoTime can't get the current time correctly, while the precision of *1000 or *1000_100 is unchanged actually. We need to rethink about the implementation way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] jixuan1989 commented on a change in pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290#discussion_r432954708
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1475,7 +1475,15 @@ long parseTimeFormat(String timestampStr) throws SQLParserException {
throw new SQLParserException("input timestamp cannot be empty");
}
if (timestampStr.equalsIgnoreCase(SQLConstant.NOW_FUNC)) {
- return System.currentTimeMillis();
+ String timePrecision = IoTDBDescriptor.getInstance().getConfig().getTimestampPrecision();
+ switch (timePrecision) {
+ case "ns":
+ return System.currentTimeMillis() * 1000_000L;
Review comment:
I think what we need is to keep the nano time as monotonic increasing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-iotdb] qiaojialin commented on a change in pull request #1290: [IOTDB-718]Fix wrong time precision of NOW()
Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #1290:
URL: https://github.com/apache/incubator-iotdb/pull/1290#discussion_r432908620
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1475,7 +1475,15 @@ long parseTimeFormat(String timestampStr) throws SQLParserException {
throw new SQLParserException("input timestamp cannot be empty");
}
if (timestampStr.equalsIgnoreCase(SQLConstant.NOW_FUNC)) {
- return System.currentTimeMillis();
+ String timePrecision = IoTDBDescriptor.getInstance().getConfig().getTimestampPrecision();
+ switch (timePrecision) {
+ case "ns":
+ return System.currentTimeMillis() * 1000_000L;
Review comment:
System.nanoTime is not the current time in nano second, it is usually used for timing:
long start = System.nanoTime();
long elapse = System.nanoTime() - start;
And, there is no System. microsecond()...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org