You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/06/22 14:40:14 UTC
[GitHub] [solr] sgift opened a new pull request, #913: SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec
sgift opened a new pull request, #913:
URL: https://github.com/apache/solr/pull/913
https://issues.apache.org/jira/browse/SOLR-16260
# Description
Adds support for Instant, LocalDate and ZonedDateTime to JavaBinCodec.
# Solution
Instant, LocalDate and ZonedDateTime are converted to epoch millis and stored as type `DATE` in the serialized bytes.
# Tests
I've added a test to TestJavaBinCodec, which tests that all three classes can be serialized and deserialized and return a Date with the same time as the input.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a diff in pull request #913: SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec
Posted by GitBox <gi...@apache.org>.
madrob commented on code in PR #913:
URL: https://github.com/apache/solr/pull/913#discussion_r916106001
##########
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##########
@@ -1080,6 +1081,16 @@ public boolean writePrimitive(Object val) throws IOException {
daos.writeByte(DATE);
daos.writeLong(((Date) val).getTime());
return true;
+ } else if (val instanceof Instant) {
+ writeStr(val.toString());
Review Comment:
do these all need a `return true` as well?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #913: SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #913:
URL: https://github.com/apache/solr/pull/913#discussion_r906209908
##########
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##########
@@ -1080,6 +1083,15 @@ public boolean writePrimitive(Object val) throws IOException {
daos.writeByte(DATE);
daos.writeLong(((Date) val).getTime());
return true;
+ } else if (val instanceof Instant) {
+ daos.writeByte(DATE);
+ daos.writeLong(((Instant) val).toEpochMilli());
+ } else if (val instanceof LocalDate) {
+ daos.writeByte(DATE);
+ daos.writeLong(((LocalDate) val).toEpochDay());
Review Comment:
I think EpochDay is the number of Days since epoch.
```suggestion
daos.writeLong(((LocalDate) val).atStartOfDay(ZoneId.systemDefault()).toEpochSecond());
```
with the `ZoneId.systemDefault()`, you might be able to support `LocalDateTime` as well.
On the other hand, we don't necessarily know what zone is correct, and the system default might be wrong. Maybe we just remove the `LocalDate` support, like you did `LocalDateTime`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #913: SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #913:
URL: https://github.com/apache/solr/pull/913#discussion_r1009603642
##########
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##########
@@ -1080,6 +1083,15 @@ public boolean writePrimitive(Object val) throws IOException {
daos.writeByte(DATE);
daos.writeLong(((Date) val).getTime());
return true;
+ } else if (val instanceof Instant) {
+ daos.writeByte(DATE);
+ daos.writeLong(((Instant) val).toEpochMilli());
+ } else if (val instanceof LocalDate) {
+ daos.writeByte(DATE);
+ daos.writeLong(((LocalDate) val).toEpochDay());
Review Comment:
Honestly I think it's better to use Date than String. Will probably revert those changes before moving forward. We would prefer this to work out of the box for most people without them having to add an UpdateProcessorFactory.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #913: SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on PR #913:
URL: https://github.com/apache/solr/pull/913#issuecomment-1164592200
@sgift this looks clean and simple! Thanks for the contribution 🙂
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #913: SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on code in PR #913:
URL: https://github.com/apache/solr/pull/913#discussion_r905203744
##########
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##########
@@ -24,6 +24,10 @@
import java.lang.invoke.MethodHandles;
import java.nio.ByteBuffer;
import java.nio.file.Path;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
Review Comment:
Did you mean to include LocalDateTime in the else/if below?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sgift commented on a diff in pull request #913: SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec
Posted by GitBox <gi...@apache.org>.
sgift commented on code in PR #913:
URL: https://github.com/apache/solr/pull/913#discussion_r905825991
##########
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##########
@@ -24,6 +24,10 @@
import java.lang.invoke.MethodHandles;
import java.nio.ByteBuffer;
import java.nio.file.Path;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
Review Comment:
I did until I noticed that I don't have enough information to convert LocalDateTime. Thanks for spotting. I've removed the import.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sgift commented on a diff in pull request #913: SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec
Posted by GitBox <gi...@apache.org>.
sgift commented on code in PR #913:
URL: https://github.com/apache/solr/pull/913#discussion_r906395456
##########
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##########
@@ -1080,6 +1083,15 @@ public boolean writePrimitive(Object val) throws IOException {
daos.writeByte(DATE);
daos.writeLong(((Date) val).getTime());
return true;
+ } else if (val instanceof Instant) {
+ daos.writeByte(DATE);
+ daos.writeLong(((Instant) val).toEpochMilli());
+ } else if (val instanceof LocalDate) {
+ daos.writeByte(DATE);
+ daos.writeLong(((LocalDate) val).toEpochDay());
Review Comment:
Thanks for spotting this and the proposed fix, but you got me thinking: Solr already has ParseDateFieldUpdateProcessorFactory to parse date strings and all JSR310 classes have a toString representation which can be parsed by it. Instead of trying to convert all to Date I've changed the code to convert them to their String representation and then ParseDateFieldUpdateProcessorFactory can parse it back on the server side. I think that's a better solution?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16260: Add support for Instant, LocalDate and ZonedDateTime to JavaBinCodec [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #913:
URL: https://github.com/apache/solr/pull/913#issuecomment-1953292408
This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org