This isn't unique to this PR, but should we be worried about injection attacks here? It seems like we should be sanitizing the inputs and probably using whatever bigquery query builder exists instead of directly munging strings from the user into our queries.

It's probably not clear from the way GitHub displays these diffs, but this PR is adding a second version of doRowsExistFor that makes its last parameter a Long instead of the String. So the changes here should be even safer than the baseline. 🙂

The code in this file is often formatting not just column values into String, but also project / dataset / table. All of these are variable in GVS (even table names, thanks to "superpartitioning" to work around BQ partitioning constraints).

At least in the GVS context I'm not particularly concerned about injection here for a few reasons:

• The project and dataset must have been validated to even create the tables in the first place.
• Table and column names are not specified from user input.
• Users would be running GVS via their pet service accounts and wouldn't have the ability to read or write anything that isn't theirs.
• These doRowsExistFor are only called to validate sample ids (Longs) and digests. So even if the VCF header had some "little Bobby Tables" in it we would be querying for the digest of that.

Happy to add some docs / warnings to clarify for potential future users for whom these assumptions may not apply.

This error message seems confusingly worded to me. Maybe something like "Non-hom-ref genotypes with no PLs are only allowed if roundContributionFromEachGenotype = true.

I find the control flow kind of confusing here now. I might rearrange it into a tree with round vs not round branches instead of pulling out that error case at the top.

Something like this:

            if (!g.hasLikelihoods()) {
                if (roundContributionFromEachGenotype) {
                    if (g.isHomRef()) {
                        genotypeWithTwoRefsCount += 1;
                    } else if (g.isHet()) {
                        genotypesWithOneRefCount += 1;
                    } else {
                        genotypesWithNoRefsCount += 1;
                    }
                } else {
                    if (!g.isHomRef()) {
                        throw new IllegalStateException("Genotype likelihoods cannot be determined from GQ for genotype: " + g +
                                ".\nGenotypes with no PLs should have integer counts using roundContributionFromEachGenotype = true.");
                    }
                    if (gq == 0) {
                        genotypeWithTwoRefsCount += 1.0/3;
                        genotypesWithOneRefCount += 1.0/3;
                        genotypesWithNoRefsCount += 1.0/3;
                    } else {
                        genotypeWithTwoRefsCount += QualityUtils.qualToProb(gq);
                        genotypesWithOneRefCount += 1 - QualityUtils.qualToProb(gq);
                        //assume last likelihood is negligible
                    }
                }
                continue;
            }

Since this was confusing enough that it was gotten wrong initially maybe add a comment explaining what this means?

It's analogous to the low_VQSLOD_ prefix - "Prefix for a site (SNP/INDEL) that failed calibration sensitivity cutoff". In this case, the site would be a failure if the sensitivity is greater than the threshold.

Instead of deleting and re-adding these, it might be better to simply move them to their destined package in this PR. That way, we could preserve the git history of changes to these utils.

This was done so I'm going to dismiss this review.