Uploaded image for project: 'OptaPlanner'
  1. OptaPlanner
  2. PLANNER-2017

groupBy(shadow entity) should throw error + support groupBy(identity(shadow entity))

    Details

    • Type: Task
    • Status: Open (View Workflow)
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: optaplanner-core
    • Labels:
      None
    • Docs QE Status:
      NEW
    • QE Status:
      NEW

      Description

      Highlights of decisions: we're implementing proposal B1 + C2 + X:

      Proposal B1) just add the warnings for CS
      +
      Proposal C2)
         groupBy(integer) // equality
          groupBy(identity(shadow variable)) // identity
          groupBy(identity(shadow variable), integer) // identity, equality
          groupBy(integer, identity(shadow variable)) // equality, identity
      +
      Proposal X) Add a TestData objects that mimics this behavior
      class TestDataHashCodeChangingEntity
        shadow var that in is hashcode
      tests:
          groupBy(shadow entity) // Identity would fix it, equality breaks it
          groupBy(integer)       // Equality would fix it, identity breaks it
          groupBy(identity(shadow variable), integer
      

      Long discussion:

      Make @PlanningId mandetory
      
      Proposal D) Detect usage of Lombok
      -1 it's not just lombok
      
      Proposal A) Do nothing.
      There are no breadcrumbs.
      
      Why isn't this a problem with Drools?
      It probably is.
      
      Proposal X) Add a TestData objects that mimics this behavior
      class TestDataHashCodeChangingEntity
        shadow var that in is hashcode
      AGREED
          groupBy(shadow entity) // Identity would fix it, equality breaks it
          groupBy(integer)       // Equality would fix it, identity breaks it
      
      
      Proposal B) Try to find all exceptions for which this would bubble up as a score corruption or a move corruption or a "impossible state" and add a line "Maybe your hashcode implementation is (...messed up)"
      "Maybe the hashCode is ..." (a few false positives are fine)
      B1) only in that CS code. Better than A). EAsy (in Uni, Bi, Tri, Quad)
      B2) also figure out for non-CS code. Avoid too many false positives. => killer, maybe just go for B1)
      
      
      Proposal C) Experiment with IdentityHashMap or such semantics
        groupBy(shadow entity) VS groupBy(integer)
        =>
        C1) groupByIdentity(shadow entity)
            groupBy(integer)
      
            multi parameters?
            groupByIdentityForFirstParam(shadow entity, integer)
      
      Theory: only have this problem for JDK class:
      groupBy(integer) = groupBy(String)
      Equality for integer, string, localtime, MyLocation(lat, long), ...
      Identity for @PlanningEntity (including shadows),
      
      ann = new Person("ann", LocalTime.of(9:00))
      beth = new Person("beth", LocalTime.of(9:00))
      groupBy(Person::getTime, count())=> requirement: 1 group: {9:00, 2}
      
      => Conclusation: equality semantics must be the default (80% rule)
      
      Should it be the only path? principle of least surprise?
      
      
      Proposal E) Make @PlanningId mandatory
      
      Proposal F) Detect if a PlanningEntity is implement equals and throw an exception if that happens
      Stronger breadcrumb than B1
      => if they don't want to change their domain objects => no optaplanner for you
      
      
      
      Proposal D) If class has @PlanningId, use that as groupBy, otherwise use equality semantics
      
      
      
      Proposal B1) just add the warnings for CS
      => if they don't want to change their domain objects => no optaplanner for you
      
      Proposal B1) + C2)
      C2) groupBy(integer) // equality
          groupBy(identity(shadow variable)) // identity
          groupBy(identity(shadow variable), integer) // identity, equality
          groupBy(integer, identity(shadow variable)) // equality, identity
      
      => error message: "maybe fix your hashcode. If you really don't want to, wrap it in Collectors.identity()"
      
      Con: suffer in scoreDRL
           I don't want to know as a user.
      
      
      Proposal E + F: force @PlanningId
      if (@PlanningEntity) {
          if (no @PlanningId) throw exception
          equality on planning Id
      } else {
          equality
      }
      Violates principle of least surprise.
      
      Version 1 of MyProject: Computer is not an PE, in version 2 Computer becomes a shadow entity
      because of some new constraint
      groupBy(Process:getComputer) from version 1.
      Violates principle of least surprise.
      
      
      Optional PlanningId + change groupBy behavior on @PlanningId detection.
      -> because behavior existing constraints changes when adding PlanningId for multithreaded solving
      
      Mandetory PlanningId => getting started is more difficult
      
      

        Gliffy Diagrams

          Marvel

          Marvel Prototype Add-On for Confluence Server written by David Simpson.

            Attachments

              Activity

                People

                • Assignee:
                  lpetrovicky Lukáš Petrovický
                  Reporter:
                  ge0ffrey Geoffrey De Smet
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  1 Start watching this issue

                  Dates

                  • Created:
                    Updated: