Z3-based SQL data generation: memoization, explicit outcomes, statistics & fixes#1586
Z3-based SQL data generation: memoization, explicit outcomes, statistics & fixes#1586agusaldasoro wants to merge 23 commits into
Conversation
|
Is this ready for review? It seems to be failing... |
@jgaleotti The changes are ready and working, but there seems to be an out of memory issue that I can not solve in the Core and CoreIT tests |
9817750 to
55cf2de
Compare
6aced03 to
d03d8ec
Compare
|
@jgaleotti Hey! It's ready for review now, thanks! |
| public void visit(Function function) { | ||
| // TODO This translation should be implemented | ||
| String name = function.getName().toUpperCase(); | ||
| if ((name.equals("LOWER") || name.equals("UPPER")) |
There was a problem hiding this comment.
replace "LOWER" and "UPPER" with constants
| /** | ||
| * Represents the outcome of a Z3 solver invocation. | ||
| * Distinguishes between three possible states: SAT (satisfiable with a model), | ||
| * UNSAT (unsatisfiable), and ERROR (solver or parsing failure). |
There was a problem hiding this comment.
what about timeout? is that an outcome?
There was a problem hiding this comment.
also, can Z3 return UNKNOWN?
| /** Non-null only when status == ERROR. */ | ||
| public final String errorMessage; | ||
|
|
||
| private Z3Result(Status status, Map<String, SMTLibValue> model, String errorMessage) { |
There was a problem hiding this comment.
create a abstract class Z3Solution and rename model to solution. (Using model can be a bit misleading nowadays due to LLMs...)
| ERROR | ||
| } | ||
|
|
||
| public final Status status; |
There was a problem hiding this comment.
why not getters and private fields?
| private val sqlRowsAverageCalculator = IncrementalAverage() | ||
|
|
||
| // DSE (Dynamic Symbolic Execution) statistics | ||
| private var dseTotalQueriesProcessed = 0 |
There was a problem hiding this comment.
To be honest, I need we need a more suitable term sonce there is no "dse/dynamic symbolic execution" involved in this work :)
What do you think of generateSqlDataWithDSE -> generateSqlDataWithZ3 ? Regarding the "dse..." it could be prefixed to "sqlZ3..." ?
| } | ||
| "NAME" -> { | ||
| assertTrue(gene is StringGene) | ||
| assertEquals("agus", (gene as StringGene).value) |
There was a problem hiding this comment.
"Alice" and "Bob" please :)
| if (config.generateSqlDataWithDSE) { | ||
| return handleDSE(ind, sampler, failedWhereQueries) | ||
| if (config.generateSqlDataWithZ3) { | ||
| return handleZ3(ind, sampler, failedWhereQueries) |
There was a problem hiding this comment.
generateSqlDataWithZ3(...)
| @@ -347,8 +347,8 @@ abstract class ApiWsStructureMutator : StructureMutator() { | |||
| return handleSearch(ind, sampler, mutatedGenes, fw) | |||
There was a problem hiding this comment.
generateSqlDataWithSearch(...)
| @@ -31,7 +31,7 @@ public void testRunEM() throws Throwable { | |||
| args.add("true"); | |||
There was a problem hiding this comment.
replace args.add with setOption(args,"generateSqlDataWithSearch","false")
| // TODO This translation should be implemented | ||
| if (dateTimeLiteralExpression.getType() == DateTimeLiteralExpression.DateTime.TIMESTAMP) { | ||
| String value = dateTimeLiteralExpression.getValue(); | ||
| if (value.startsWith("'") && value.endsWith("'")) { |
There was a problem hiding this comment.
replace "'" with constant
| // Treat the timestamp string as UTC to match the UTC-based decoder in | ||
| // SMTLibZ3DbConstraintSolver (LocalDateTime.ofInstant(..., UTC)). | ||
| long epochSeconds = java.time.LocalDateTime.parse(value, | ||
| DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")) |
There was a problem hiding this comment.
replace "yyyy-MM-dd HH:mm:ss" with constant
| String stderr = result.getStderr(); | ||
| throw new RuntimeException("No result after solving file " + stderr); | ||
| String trimmed = stdout.trim(); | ||
| if (trimmed.startsWith("unsat")) { |
There was a problem hiding this comment.
replace "unsat" with constant
| // "unknown" means Z3 could not decide (e.g. incomplete theory or timeout). | ||
| // It must be handled explicitly: otherwise it would fall through and be parsed | ||
| // as an empty model, silently masquerading as SAT. | ||
| if (trimmed.startsWith("unknown")) { |
There was a problem hiding this comment.
replace "unknown" with constant
| if (trimmed.startsWith("unknown")) { | ||
| return Z3Result.unknown(); | ||
| } | ||
| if (!trimmed.startsWith("sat")) { |
There was a problem hiding this comment.
replace "sat" with constant
| return Z3Result.error("Unexpected Z3 output for file " + fileName + ": " + stdout); | ||
| } | ||
|
|
||
| Map<String, SMTLibValue> assignments = SMTResultParser.parseZ3Response(stdout); |
There was a problem hiding this comment.
The parser should create a solution instead of a assignment map
| /** | ||
| * @return the assignments of this solution, keyed by variable/constant name. | ||
| */ | ||
| public abstract Map<String, SMTLibValue> getAssignments(); |
| // Defensive: Z3DockerExecutor already classifies 'unsat'/'unknown' before invoking this | ||
| // parser, so in practice only 'sat' models reach here. This guard is kept as a safety net | ||
| // in case the parser is ever called directly with an unsat response. | ||
| if (line.startsWith("unsat")) { |
There was a problem hiding this comment.
replace "unsat" with constant value
| smt.addNode(constraint) | ||
| } | ||
| } catch (e: RuntimeException) { | ||
| LoggingUtil.getInfoLogger().warn("Could not translate WHERE clause to SMT-LIB, skipping: ${where}. Reason: ${e.message}") |
There was a problem hiding this comment.
is this failure considered in the sqlZ3 stats?
There was a problem hiding this comment.
Good catch! It wasn't (the catch is inside generateSMT, so the predicate is silently dropped and Z3 still returns SAT).
Now fixed: it increments skippedQueryConstraints (here + the JOIN ON catch) and is reported as a new sqlZ3PartialTranslations stat column.
| * from the average, so they do not distort the correctness metric. | ||
| */ | ||
| val distResult = computeCorrectnessDistance(sqlQuery, schemaDto, sqlActions) | ||
| if (distResult.sqlDistanceEvaluationFailure) { |
There was a problem hiding this comment.
is this considered in the stats?
| val distResult = computeCorrectnessDistance(sqlQuery, schemaDto, sqlActions) | ||
| if (distResult.sqlDistanceEvaluationFailure) { | ||
| LoggingUtil.getInfoLogger().warn("SQL-Z3: correctness evaluation failure for query '$sqlQuery'") | ||
| } else if (distResult.sqlDistance != 0.0) { |
There was a problem hiding this comment.
is this case considered in the stats?
There was a problem hiding this comment.
Yes. Both branches are recorded by the reportSqlZ3CorrectnessDistance(distResult.sqlDistance, distResult.sqlDistanceEvaluationFailure) call right below this block.
arcuri82
left a comment
There was a problem hiding this comment.
@agusaldasoro hi, this was a large PR, touching different aspects. please try to make smaller, more focused PRs in future :) eg, changing of surefire setting could had been done independently on a different PR
| } | ||
|
|
||
| // correctness distance stats (only emitted when measureSqlZ3Correctness=true) | ||
| if (config.measureSqlZ3Correctness) { |
There was a problem hiding this comment.
why 2 distinct configuration options? as we do this only for debugging, could just have 1
There was a problem hiding this comment.
These correctness stats were something I added to debug the code for myself — the idea was to run them locally only (not on the cluster). They were genuinely useful: they helped me find a few bugs
by measuring how far the generated rows were from actually satisfying the predicate. But they're not needed for the time-measurement runs we do on the cluster, and they add overhead that could
interfere with the real objective there — which is exactly why they were a separate flag from collectSqlZ3Stats.
That said, to keep this PR focused I've moved the whole correctness feature to a separate branch (dse/sql-z3-correctness) so we can discuss it later. It's not needed for now, so this PR no longer
has that second option.
| for (join in joins) { | ||
| val onExpressions = join.onExpressions | ||
| if (onExpressions.isNotEmpty()) { | ||
| // KNOWN LIMITATION: only the first ON expression is used; a composite ON |
There was a problem hiding this comment.
should add a TODO in the comments here
| try { | ||
| val condition = parser.parse(onExpression.toString(), toDBType(schema.databaseType)) | ||
| val tableFromQuery = TablesNamesFinder().getTables(sqlQuery as Statement).first() | ||
| // KNOWN LIMITATION: the ON condition is translated with the SAME row index on |
There was a problem hiding this comment.
should add a TODO in the comments here
| @@ -539,37 +608,62 @@ class SmtLibGenerator(private val schema: DbInfoDto, private val numberOfRows: I | |||
|
|
|||
| companion object { | |||
There was a problem hiding this comment.
companion objects should be top of file, and not after line 600
| companion object { | ||
|
|
||
| // Maps database column types to SMT-LIB types | ||
| // Bounds for TIMESTAMP columns, encoded as epoch seconds (SMT Int). |
There was a problem hiding this comment.
use JavaDoc /** */ for fields and functions
| * Extracts the table name from the key by removing the last character (index). | ||
| * Extracts the table name from a row-constant key by removing the trailing row index. | ||
| * | ||
| * Row constants are named "${smtName}${i}" (e.g. "users1", "users2"). The trailing digits are |
There was a problem hiding this comment.
did you choose this? if so, wouldn't better to have a separator? eg, ${smtName}__${i}
| /** | ||
| * Maps column types to ColumnDataType. | ||
| * | ||
| * FUTURE WORK: this recognizes only a small subset of SQL type spellings and falls back to |
There was a problem hiding this comment.
put TODO. note that TODO and FIXME comments are handled specially in IDEs
| "Distance=0 means the insert satisfies the WHERE; distance>0 means it does not. " + | ||
| "Only meaningful when generateSqlDataWithZ3=true.") | ||
| @DependsOnTrueFor("generateSqlDataWithZ3") | ||
| var measureSqlZ3Correctness = false |
There was a problem hiding this comment.
no need of 2 separated options to collect statistics
| </dependency> | ||
| <dependency> | ||
| <groupId>org.evomaster</groupId> | ||
| <artifactId>evomaster-client-java-sql</artifactId> |
There was a problem hiding this comment.
you should not add this kind of dependency between modules before discussing it, as it has several architectural consequences. remove it. different though if it is imported as test
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <configuration> | ||
| <argLine>@{argLine} -ea -Xms1024m -Xmx5120m -Xss4m -Dfile.encoding=UTF-8 -Djdk.attach.allowAttachSelf=true -Duser.language=en -Duser.country=GB ${addOpens}</argLine> |
There was a problem hiding this comment.
you shouldn't copy&paste configurations like this, as it ends up with code duplication, which must be avoided, as the 2 will inevitably diverge throught time. why not increasing -Xms directly on the root pom avoiding duplication? otherwise, that value itself should be in a param, which is the overriden in properties declaration in this pom file
Summary
Adds memoization and explicit outcome handling to EvoMaster's Z3-based SQL data generation (
generateSqlDataWithZ3), plus optional statistics/correctness instrumentation and several encoding fixes.(sqlQuery, numberOfRows)in a bounded LRU, skipping redundant Docker/Z3 executions for repeated queries. Only deterministic outcomes (SAT/UNSAT) are cached; UNKNOWN/ERROR are not (they may be transient). The cache stores theZ3Result(not the generatedSqlActions), so each individual still gets fresh, independently-mutable genes.Z3Result(SAT/UNSAT/UNKNOWN/ERROR) andZ3Solution, replacingOptional<Map<String, SMTLibValue>>.Z3DockerExecutorclassifies Z3's stdout before parsing, sounknown/unsatare no longer misparsed as an empty SAT model.sqlZ3TimeoutMs, default 5000ms): passes a soft-t:timeout to Z3; on expiry Z3 returnsunknown(mapped toUNKNOWN) instead of running unbounded.sqlZ3NumberOfRows, default 1): number of rows generated per table.collectSqlZ3Stats): per-run counters written tostatistics.csvonly when enabled — query volume & cache reuse, solver outcomes, and cost (Z3 time, SMT-LIB gen time/size).measureSqlZ3Correctness): after a SAT result, generated rows are re-evaluated withSqlHeuristicsCalculatorto measure their distance to the original SQL predicate; mismatches are logged and recorded.SmtLibGeneratorsupportsDELETEandUPDATE, not justSELECT.LongGeneinstead of truncating via.toInt().JSqlVisitor(LOWER/UPPER, timestamp literals) and addedSmtLibGeneratorTestcoverage.Statisticsvia aWeakReference, so its@PreDestroyregistration (a GC root through Governator's predestroy-monitor thread) no longer pinsStatistics → Archive → individuals, which was OOMing test suites that build many injectors.All new options are
@Experimentaland default-off (or default 1 / 5000), so default runs are unaffected.Configuration
generateSqlDataWithZ3sqlZ3NumberOfRowssqlZ3TimeoutMscollectSqlZ3StatsmeasureSqlZ3CorrectnessStatistics (
collectSqlZ3Stats)sqlZ3QueriesSeen,sqlZ3UniqueQueries,sqlZ3DuplicateQueries,sqlZ3CacheHits,sqlZ3CacheMisses— invariant:sqlZ3QueriesSeen = sqlZ3CacheHits + sqlZ3CacheMisses.sqlZ3Sat,sqlZ3Unsat,sqlZ3Unknown,sqlZ3Errors,sqlZ3ParseFailures— sum equalssqlZ3CacheMisses.sqlZ3TotalMs,sqlZ3SmtlibGenTotalMs,sqlZ3AvgSmtlibSizeBytes.Correctness (
measureSqlZ3Correctness)sqlZ3CorrectnessChecks,sqlZ3CorrectnessZeroDistance,sqlZ3CorrectnessNonZero,sqlZ3CorrectnessAvgDist,sqlZ3CorrectnessEvalFailures.Known limitations (documented in code as
KNOWN LIMITATION/FUTURE WORK)ONuses diagonal row pairing (rowi↔ rowi) and only the firstONconjunct — sufficient atk=1, not full INNER JOIN semantics.IS NULL/IS NOT NULL/= NULL) are dropped; all columns are treated asNOT NULL.NUMERICmaps toInt(truncating decimals), inconsistent withDECIMAL → Real.ColumnDto.typeindependently and could drift on variant spellings.