You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/11/07 23:26:12 UTC

[GitHub] [druid] clintropolis opened a new pull request #11887: add missing json type for ListFilteredVirtualColumn

clintropolis opened a new pull request #11887:
URL: https://github.com/apache/druid/pull/11887


   ### Description
   #11650 missed adding JSON type info for `ListFilteredVirtualColumn`, which results in failure when actually trying to use it. This was my bad - it had serde tests, but not serde as a part of a query, so there was a blind spot.
   
   I've added a JSON serialization round trip as part of validation in `BaseCalciteQueryTest` to try to prevent this from happening again.
   
   I suspect that not everything on every query implements `equals` fully, because directly comparing the round-tripped serialized query against the expected query resulted in a failure, but comparing the string of the round tripped query against the string of the expected query does seem to pass, so went with that for now. All of the `MV_FILTER_*` tests failed prior to this change, so it should at least help.
   
   
   <hr>
   
   <!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
   
   This PR has:
   - [x] been self-reviewed.
   - [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
   - [ ] been tested in a test Druid cluster.
   


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis commented on pull request #11887: add missing json type for ListFilteredVirtualColumn

Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11887:
URL: https://github.com/apache/druid/pull/11887#issuecomment-963540318


   I ended up getting the round trip JSON serde tests working by just creating a fresh `ObjectMapper`, which is sad, and requires subclasses of `BaseCalciteQueryTest` to override a new method it has, `getJacksonModules` which can add the jackson modules to both the static shared mapper and the mapper created specifically for the round trip tests.
   
   I'm unsure if this is worth the trouble, but it does seem useful still to catch things like this so I think the answer is barely yes.
   
   I still don't quite understand why the static object mapper doesn't work correctly when running the datasketches extension calcite tests. Examining the mapper in the debugger I see that all of the subtypes are present, but it still serializes to string incorrectly with the class name instead of the type names specified in `SketchModule` and `DoublesSketchModule`, only `HllSketchModule` works correctly (it runs first).


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis edited a comment on pull request #11887: add missing json type for ListFilteredVirtualColumn

Posted by GitBox <gi...@apache.org>.
clintropolis edited a comment on pull request #11887:
URL: https://github.com/apache/druid/pull/11887#issuecomment-962870550


   having some trouble getting this round trip serde testing to actually work with all of the extensions; for some reason doubles sketch and theta sketch sql tests are failing, even though they are registering the jackson modules... 
   
   I am about to give up and just add coverage the thing that I'm fixing :cry:


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis commented on pull request #11887: add missing json type for ListFilteredVirtualColumn

Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11887:
URL: https://github.com/apache/druid/pull/11887#issuecomment-962870550


   having some trouble getting this round trip serde testing to actually work with all of the extensions: for some reason doubles sketch and theta sketch sql tests are failing, even though they are registering the jackson modules... 
   
   I am about to give up and just add coverage the thing that I'm fixing :cry:


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis merged pull request #11887: add missing json type for ListFilteredVirtualColumn

Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #11887:
URL: https://github.com/apache/druid/pull/11887


   


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org