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

HSEARCH-3099 HSEARCH-3553 Restore support for "indexNullAs" in @Field #1947

Merged
merged 17 commits into from Apr 23, 2019

Conversation

Projects
None yet
3 participants

@fax4ever fax4ever added the Preview label Apr 8, 2019

@coveralls

This comment has been minimized.

Copy link

commented Apr 8, 2019

Pull Request Test Coverage Report for Build 21

  • 255 of 290 (87.93%) changed or added relevant lines in 74 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.008%) to 88.564%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mapper/pojo/src/main/java/org/hibernate/search/mapper/pojo/bridge/builtin/impl/DefaultJavaNetURIValueBridge.java 3 4 75.0%
mapper/pojo/src/main/java/org/hibernate/search/mapper/pojo/bridge/builtin/impl/DefaultJavaNetURLValueBridge.java 3 4 75.0%
engine/src/main/java/org/hibernate/search/engine/cfg/spi/ValidateUtils.java 22 25 88.0%
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/index/admin/impl/ElasticsearchSchemaValidatorImpl.java 0 5 0.0%
engine/src/main/java/org/hibernate/search/engine/cfg/spi/ParseUtils.java 54 59 91.53%
engine/src/main/java/org/hibernate/search/engine/cfg/spi/ConvertUtils.java 32 52 61.54%
Totals Coverage Status
Change from base Build 215: -0.008%
Covered Lines: 16914
Relevant Lines: 19098

💛 - Coveralls

@fax4ever fax4ever force-pushed the fax4ever:3099-make-index-null-as branch 5 times, most recently from 0d5abcb to c14df18 Apr 9, 2019

@fax4ever fax4ever removed the Preview label Apr 10, 2019

@fax4ever fax4ever force-pushed the fax4ever:3099-make-index-null-as branch from f4823d1 to 638c76d Apr 10, 2019

@fax4ever fax4ever changed the title HSEARCH-3058 Restore support for "indexNullAs" in @Field HSEARCH-3058 HSEARCH-3553 Restore support for "indexNullAs" in @Field Apr 10, 2019

@yrodiere
Copy link
Member

left a comment

My comments below. I disagree with a few details in the backend side of the implementation, but in my opinion the most important problem is the lack of coverage and extensibility of the mapper side of the implementation: it limits support to just a few primitive types, even though we used to support much more in Search 5, and it completely bars the way to users defining their own parsing method for custom bridges.

@fax4ever fax4ever changed the title HSEARCH-3058 HSEARCH-3553 Restore support for "indexNullAs" in @Field HSEARCH-3099 HSEARCH-3553 Restore support for "indexNullAs" in @Field Apr 15, 2019

@fax4ever fax4ever force-pushed the fax4ever:3099-make-index-null-as branch 3 times, most recently from f3e3307 to 8675b7e Apr 16, 2019

@fax4ever fax4ever force-pushed the fax4ever:3099-make-index-null-as branch 2 times, most recently from c3e8fc8 to f991d89 Apr 17, 2019

@fax4ever fax4ever force-pushed the fax4ever:3099-make-index-null-as branch from f991d89 to a3506f5 Apr 18, 2019

@fax4ever

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@yrodiere, thanks for the in depth review. I think I've addressed most of the suggestions.

@yrodiere
Copy link
Member

left a comment

Nearly there!

I have a few optional comments below. Feel free to ignore them, but one thing that is still missing is the documentation.

I can do it if you prefer, but I think we should add:

  • Something about indexNullAs in mapping annotations. See mapper-orm-mapping.asciidoc, section "Direct field mapping". You can add an item in the definition list at the very bottom of that section, after "projectable". It's not worth your time to add an example, one or two sentences should be enough since the feature is rather straightforward (to use... I know it's been hard to implement :) ).
  • Something about how indexNullAs in mapping annotations is parsed for each built-in value bridge. See mapper-orm-mapping.asciidoc, section "Built-in value bridges". You can add a column "Parsing method for 'indexNullAs'" to the right of the table. Personally I would simply put something like ZoneOffset.parse(String) in most cases.

@fax4ever fax4ever force-pushed the fax4ever:3099-make-index-null-as branch from 1a573dd to 3c9e717 Apr 19, 2019

@fax4ever

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

Thank you @yrodiere. Is there other we can improve?

@fax4ever fax4ever force-pushed the fax4ever:3099-make-index-null-as branch from 3c9e717 to 7b19987 Apr 19, 2019

@yrodiere

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

It looks good; I pushed a few commits with minor fixes and will merge as soon as CI goes green.

@fax4ever

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Hi @yrodiere! Seen the fixes, they are good for me. Thank you ;)

yrodiere added some commits Apr 23, 2019

HSEARCH-3099 Use a standard format for indexNullAs for ZoneOffset, Du…
…ration, Period

Rather than exposing the internal format.

This is consistent with what we do for the character bridge in
particular, or the URI/URL bridges, or the Enum bridge, or the ZoneId
bridge: we expect a string representing the type seen from the user
side, not the indexed type.

@yrodiere yrodiere force-pushed the fax4ever:3099-make-index-null-as branch from 5ecebd4 to e2a158a Apr 23, 2019

@yrodiere yrodiere merged commit e2a158a into hibernate:master Apr 23, 2019

1 check was pending

continuous-integration/jenkins/pr-merge This commit is being built
Details
@yrodiere

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Merged. Thanks a lot!

@fax4ever

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.