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

SQL: Fix incorrect message for aliases #31792

Merged
merged 3 commits into from
Jul 5, 2018
Merged

Conversation

costin
Copy link
Member

@costin costin commented Jul 4, 2018

Fix the naming in the verification message thrown for aliases over
multiple indices with different mappings.

Closes #31611
Closes #31784

Fix the naming in the verification message thrown for aliases over
multiple indices with different mappings.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@Test
public void testMergeDifferentMapping() throws Exception {
Map<String, EsField> oneMapping = TypesTests.loadMapping("mapping-basic.json", true);
Map<String, EsField> sameMapping = TypesTests.loadMapping("mapping-basic.json", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add one more test for two mappings that are logically the same (same field types, same field names) but the jsons themselves are different (different fields order for example)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES mapping are always returned in order (which is important since the underlying Maps depend on it). Hence why the loadMapping has the true parameter otherwise the order is intentionally randomized (which causes the map equality to fail).

IndexResolution resolution = IndexResolver.merge(
Arrays.asList(IndexResolution.valid(new EsIndex("a", oneMapping)),
IndexResolution.valid(new EsIndex("b", sameMapping)),
IndexResolution.valid(new EsIndex("diff", differentMapping))),
Copy link
Contributor

@astefan astefan Jul 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth putting the different mapping as first in the List as a second test? (EsIndex("diff"...), EsIndex("same"..), EsIndex("one"))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order is somewhat important for this test since the comparison is done between the first entry and the rest.
The order above tests that a and b pass and that a and diff fail - there are two comparisons applied.
Putting diff first will cause only one comparison since and and b are not compared.
A minor detail that was done on purpose.

@davidkyle davidkyle added v6.3.2 and removed v6.3.1 labels Jul 5, 2018
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So long as @astefan likes it I'm happy.

@costin costin merged commit 6e9bd26 into elastic:master Jul 5, 2018
@costin costin deleted the fix-for-31611 branch July 5, 2018 16:50
@costin
Copy link
Member Author

costin commented Jul 5, 2018

Thanks for reviewing folks!

costin added a commit that referenced this pull request Jul 5, 2018
Fix the naming in the verification message thrown for aliases over
multiple indices with different mappings.
costin added a commit that referenced this pull request Jul 5, 2018
Fix the naming in the verification message thrown for aliases over
multiple indices with different mappings.
dnhatn added a commit that referenced this pull request Jul 5, 2018
* 6.x:
  Test: Do not remove xpack templates when cleaning (#31642)
  SQL: Allow long literals (#31777)
  SQL: Fix incorrect message for aliases (#31792)
  Detach Transport from TransportService (#31727)
  6.3.1 release notes (#31829)
  Add unreleased version 6.3.2
  [ML][TEST] Use java 11 valid time format in DataDescriptionTests (#31817)
  [ML] Don't treat stale FAILED jobs as OPENING in job allocation (#31800)
  [ML] Fix calendar and filter updates from non-master nodes (#31804)
  Fix license header generation on Windows (#31790)
  mark XPackRestIT.test {p0=monitoring/bulk/10_basic/Bulk indexing of monitoring data} as AwaitsFix
  Add JDK11 support without enabling in CI (#31644)
  Watcher: Fix check for currently executed watches (#31137)
  [DOCS] Fixes 6.3.0 release notes (#31771)
  Watcher: Ensure correct method is used to read secure settings (#31753)
  [ML] Rate limit established model memory updates (#31768)
  SQL: Update CLI logo
dnhatn added a commit that referenced this pull request Jul 5, 2018
* master:
  REST high-level client: add get index API (#31703)
  SQL: Allow long literals (#31777)
  SQL: Fix incorrect message for aliases (#31792)
  Test: Do not remove xpack templates when cleaning (#31642)
  Reduce more raw types warnings (#31780)
  Add unreleased version 6.3.2
  Scripting: Remove support for deprecated StoredScript contexts (#31394)
  [ML][TEST] Use java 11 valid time format in DataDescriptionTests (#31817)
  [ML] Don't treat stale FAILED jobs as OPENING in job allocation (#31800)
  [ML] Fix calendar and filter updates from non-master nodes (#31804)
  Fix license header generation on Windows (#31790)
  mark RollupIT.testTwoJobsStartStopDeleteOne as AwaitsFix
  mark SearchAsyncActionTests.testFanOutAndCollect as AwaitsFix
  Correct exclusion of test on JDK 11
  Fix doclint jdk 11
  Add JDK11 support and enable in CI (#31644)
  Watcher: Fix check for currently executed watches (#31137)
  Watcher: Ensure correct method is used to read secure settings (#31753)
  SQL: Update CLI logo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants